Bug 46915 – Windows client needs updating when live iframe element is moved between pages

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Jenn Braithwaite]

fast/frames/iframe-reparenting-adopt-node.html crashes due to this bug.  The Windows crash log can be found here: http://build.webkit.org/results/Windows%20Release%20(Tests)/r68785%20(4613)/CrashLog_0ad0_2010-09-30_09-27-13-421.txt

The WebFrame needs to update its pointer to the WebView in WebFrameLoaderClient::didTransferChildFrameToNewDocument().  It is still pointing to the WebView of the former page, which has been deleted in this test. Accessing the invalid pointer results in the crash. 

This bug is similar to 46300 in the GTK client.

[Adam Roben (:aroben)]

I believe bug 44713 is the Mac equivalent.

[Adam Roben (:aroben)]

<rdar://problem/8497488>

[Jenn Braithwaite]

(In reply to comment #1)
> I believe bug 44713 is the Mac equivalent.

44713 has a slightly different issue.  Mac has the correct WebView for the frame after reparenting, but the resource is only known to the WebView of the former page.  Knowledge of the frame's resources need to be transferred over from the old WebView to the frame's current WebView to fix 44713.

After this bug is fixed for Windows, we also have to fix 44713 for Windows (and other platforms).

[Jenn Braithwaite]

Created attachment 71226 [details]
patch

[Adam Roben (:aroben)]

Comment on attachment 71226 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71226&action=review

> WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:742
>  void WebFrameLoaderClient::didTransferChildFrameToNewDocument(Page*)
>  {
> +    WebView* webView = m_webFrame->webView();
> +    Frame* coreFrame = core(m_webFrame);
> +    Frame* coreParentFrame = coreFrame->tree()->parent();
> +    WebView* parentWebView = kit(coreParentFrame)->webView();
> +
> +    if (webView != parentWebView)
> +        m_webFrame->setWebView(parentWebView);
> +
> +    ASSERT(m_webFrame->webView()->page() == coreFrame->page());
>  }

Does the passed-in Page correspond to the new document? If so you can just do:

WebView* newWebView = core(page);

(The name of this function is confusing; it's talking about moving a frame between pages, but says "document" instead of "page"!)

[Jenn Braithwaite]

(In reply to comment #5)
> (From update of attachment 71226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71226&action=review
> 
> > WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:742
> >  void WebFrameLoaderClient::didTransferChildFrameToNewDocument(Page*)
> >  {
> > +    WebView* webView = m_webFrame->webView();
> > +    Frame* coreFrame = core(m_webFrame);
> > +    Frame* coreParentFrame = coreFrame->tree()->parent();
> > +    WebView* parentWebView = kit(coreParentFrame)->webView();
> > +
> > +    if (webView != parentWebView)
> > +        m_webFrame->setWebView(parentWebView);
> > +
> > +    ASSERT(m_webFrame->webView()->page() == coreFrame->page());
> >  }
> 
> Does the passed-in Page correspond to the new document? If so you can just do:
> 
> WebView* newWebView = core(page);
> 
> (The name of this function is confusing; it's talking about moving a frame between pages, but says "document" instead of "page"!)

The passed in page is the old page.  I will be removing that argument after bug 44713 is fixed as it turns out that argument is not needed for the fix.

[Adam Roben (:aroben)]

Comment on attachment 71226 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71226&action=review

>>> WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:742
>>>  }
>> 
>> Does the passed-in Page correspond to the new document? If so you can just do:
>> 
>> WebView* newWebView = core(page);
>> 
>> (The name of this function is confusing; it's talking about moving a frame between pages, but says "document" instead of "page"!)
> 
> The passed in page is the old page.  I will be removing that argument after bug 44713 is fixed as it turns out that argument is not needed for the fix.

This code will crash if coreFrame doesn't have a parent frame, or that parent frame doesn't have a WebFrame associated with it. You should either add some assertions or some null-checks. Other than that this looks fine.

[Jenn Braithwaite]

Created attachment 71605 [details]
updated patch

Revised to update the webview without referring to the parent frame as it's possible to find the webview directly from Page. Same result. Simpler code.

[Adam Roben (:aroben)]

Comment on attachment 71605 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71605&action=review

> WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:737
>  void WebFrameLoaderClient::didTransferChildFrameToNewDocument(Page*)
>  {
> +    Frame* coreFrame = core(m_webFrame);
> +    if (m_webFrame->webView()->page() != coreFrame->page())
> +        m_webFrame->setWebView(kit(coreFrame->page()));
>  }
>  

This definitely looks better. It's possible for a Frame not to have a Page, and for a WebFrame not to have a WebView, though. Could we run into that situation here?

[Jenn Braithwaite]

Created attachment 71613 [details]
updated patch again

Revised code to work even if Frame has no Page - kit(Page) handles this case - and if WebFrame has no WebView.

[WebKit Commit Bot]

Comment on attachment 71613 [details]
updated patch again

Clearing flags on attachment: 71613

Committed r70376: <http://trac.webkit.org/changeset/70376>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.