Created attachment 240529 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Created attachment 240574 [details] Patch Fixed that.

Comment on attachment 240574 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240574&action=review This is basically a new feature, and I think it'd be important to test cross platform. > Source/WebCore/history/CachedFrame.cpp:107 > + if (frame.didDisplayInsecureContent()) { How is it okay to check this on the frame when (in the common main-frame case) it is being reused from the previously shown content? > Source/WebCore/history/CachedFrame.cpp:112 > + if (frame.didRunInsecureContent()) { Ditto. > Source/WebCore/history/CachedFrame.cpp:114 > + frame.loader().client().didRunInsecureContent(m_document->securityOrigin(), m_url); "did run insecure content" being overloaded for the page cache restore case seems weird. > Source/WebCore/loader/FrameLoader.cpp:-1805 > + dispatchDidCommitLoad(); > + > + // CachedPage::restore() emits events (e.g. didDisplayInsecureContent) that are good to have after didCommitLoad. > // FIXME: This API should be turned around so that we ground CachedPage into the Page. > cachedPage->restore(*m_frame.page()); > > - dispatchDidCommitLoad(); Did you run the WK2 API tests before making this change? I suspect they will be very relevant. Also, as is suggested in this code and your comment, FrameLoader is usually incharge of dispatching events and callbacks etc etc. Yet now some of that logic is being moved into the cached frame restore. That doesn't seem good. > Source/WebCore/page/Frame.h:269 > + void setDidDisplayInsecureContent(bool value = true) { m_didDisplayInsecureContent = value; } > + bool didDisplayInsecureContent() const { return m_didDisplayInsecureContent; } > + void setDidRunInsecureContent(bool value = true) { m_didRunInsecureContent = value; } > + bool didRunInsecureContent() const { return m_didRunInsecureContent; } Why is Frame the right place for these? Frames are commonly reused between navigations (and the main frame always is) I'm not confident the values are kept fully up to date in this patch. It would be much better to attach these values to something that *does* change with navigations. Document is one example. DOMWindow is another. etc etc

Thanks for the review. I'll work on cross-platform tests before I try to address your comments. (In reply to comment #4) > This is basically a new feature, and I think it'd be important to test cross > platform. I'd call it a minor security bug rather than a feature, since it's unsafe for a port to use these events unless it completely blocks all mixed content. > > Source/WebCore/history/CachedFrame.cpp:107 > > + if (frame.didDisplayInsecureContent()) { > > How is it okay to check this on the frame when (in the common main-frame > case) it is being reused from the previously shown content? You're right; now loading a single page with insecure content causes unrelated future loads from the page cache to emit these events, like you suspected. That's very bad. I'll add a test for this. > Did you run the WK2 API tests before making this change? I suspect they > will be very relevant. If we're thinking of the same tests (the Google framework ones), those all passed.

Created attachment 245906 [details] Patch

Brady, thanks for the last review and sorry for the delay on this. I think this second attempt addresses your previous comments. Carlos might want to approve the GTK-specific changes.

Strangely, the symptom didn't reproduce in Safari when I tested going back to <https://www.ssllabs.com/ssltest/viewMyClient.html> - it just won't go into page cache. I wonder if this is something that we do intentionally.

I did consider rejecting the page from the page cache as a potential solution for this (and the equivalent favicon cache issue). The disadvantage is that would obviously make affected sites slower. OTOH, such sites are broken, and I'm fine with them receiving a performance penalty for that.

Comment on attachment 245906 [details] Patch I think my patch should store a list of insecure URIs so that they can be reported properly, rather than pretending that the entire frame is insecure.

*** Bug 134882 has been marked as a duplicate of this bug. ***

(In reply to Michael Catanzaro from comment #11) > *** Bug 134882 has been marked as a duplicate of this bug. *** This (private) bug has another testcase.

Hm, I thought this might be fixed by r206006, but the reproducer in my first comment is still failing so maybe not.

If we fix bug #219396, then we can close this issue as obsolete because the page cache should never contain insecure content.

This is obsoleted by bug #247197 and bug #219396.