[Webkit-unassigned] [Bug 138127] Insecure content warnings not emitted when page is restored from page cache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=138127

Brady Eidson <beidson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #240574|review?                     |review-
              Flags|                            |

--- Comment #4 from Brady Eidson <beidson at apple.com> ---
Comment on attachment 240574
  --> https://bugs.webkit.org/attachment.cgi?id=240574
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141030/efed43d3/attachment-0002.html>