Bug 44713 – Resource tracking failure when trying to move a frame between documents

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

Created attachment 65610 [details]
test case (will crash)

Resource identifiers are per-WebView, so when a frame is moved between documents, they can get out of sync. The attached test case crashes with an assertion (there seem to be further problems in other code, but let's track only this issue here).

[Alexey Proskuryakov]

<rdar://problem/8359858> tracks issues seen on this test case for Apple.

[Jenn Braithwaite]

With each platform doing different things with the resource identifier, I have to make platform-specific fixes. All fixes will be along these lines:

* The client impls will update their resource tracking in FrameLoaderClient::didTransferChildFrameToNewDocument(). In that method, the code needs access to both the old parent and the new parent, e.g. for mac, the WebView. (I'm not sure the old parent is always available at this point - I may have to make it available somehow - more research needed.)
* Add API for the embedder to remove the resource id from the old parent and add it to the new.  The embedder may store additional info about resources being loaded, so the API should allow the info to be transferred to the new parent. The new parent then generates a new id for the resource. E.g. for mac, the API is added to WebResourceLoadDelegate. 
* Add API to WebCore::DocumentLoader to make the identifier and original request info available for both main and sub resource loads. I'm thinking of a DocumentLoader::forEachLoadingResource(callback) method, where the callback is given the identifier, DocumentLoader* and ResourceRequest& as const params.

Does this sound like the right approach for the fix?

[Alexey Proskuryakov]

Another option may be to make resource tracking global (not per-WebView). I'm not sure what the tradeoffs are.

[Brady Eidson]

(In reply to comment #3)
> Another option may be to make resource tracking global (not per-WebView). I'm not sure what the tradeoffs are.

I'm also not sure what the tradeoffs are, but I suspect it'd be a win overall in cleaner concept, behavior, and code.

[Jenn Braithwaite]

Looked into global resource tracking - I'm not sure it makes the fix cleaner. Global resource tracking makes it cleaner for WebView to add/remove/find identifiers, even those it didn't create, however, the identifier is mapped to an object id that is generated by the ResourceLoadDelegate. One WebView cannot safely use the object id created by the ResourceLoadDelegate of a different WebView, so the old and new ResourceLoadDelegates still need to be updated about resource loads that now belong to a different page. This needs to happen at time of re-parenting in case there's some UI about the resource load on the former page (e.g. progress indicator).

A global resource tracking table would also be more complex than the current mapping of identifier->object-id. The resource identifier is not unique across WebViews so the table index would need another component to make it unique. The table value's would also need to be more than just the object id; we'll also need to know which WebView/ResourceLoadDelegate created the object id.

Essentially, you end up with the same changes for the fix, along with a more complicated table.

[Jenn Braithwaite]

The resource ids need to be made globally unique across all Pages (and thus unique across WebViews) to allow passing the ids between WebViews. Another option is to generate a new id for the resource when it is re-assigned to a different WebView, but I'm not sure it's safe to change the resource identifier after the resource has started loading.

Any comments on either approach?

[Alexey Proskuryakov]

The most interesting part here is likely making sure that Web Inspector behaves reasonably.

[Adam Roben (:aroben)]

Note that fast/frames/iframe-reparenting-adopt-node.html is being skipped on both Mac and Windows due to this bug.

[Adam Roben (:aroben)]

A 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

[Jenn Braithwaite]

(In reply to comment #9)
> A 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


Thanks for the heads up.  This crash is due to a bug similar to 46300.  The WebFrame instance is still pointing to the WebView of the former page.  I'll enter a separate bug for it.

[Jenn Braithwaite]

Created attachment 70668 [details]
patch

The fix contains the framework for all WebKit clients to update resource tracking, but only fixes the mac case. Other platforms will be fixed in separate patches.

The test case attached to this bug has been added as a LayoutTest in the patch.

[Jenn Braithwaite]

Comment on attachment 70668 [details]
patch

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

I will upload a new patch with the trigger for updating resource tracking moved within WebCore.

> WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1395
> +    loader->transferLoadingResourcesToNewPage(oldPage);

Trigger for updating resource tracking needs to be moved to WebCore beause there are other webcore components that need to update resource tracking.

[Jenn Braithwaite]

Created attachment 70760 [details]
updated patch

Moved the trigger to update resource tracking into WebCore.

[Dmitry Titov]

Comment on attachment 70760 [details]
updated patch

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

Some preliminary comments. It would be good if a reviewer closer to Apple mac port could take a look.

A couple of naming and other small nits:

> LayoutTests/fast/frames/iframe-reparenting-fail-load.html:21
> +<p>This test adopts an iframe into a second page, then closes that page, cancelling that iframe load.</p>

It would be good to add a short description of actual failure this test is trying to reproduce, with a note on how to see if the test succeeds or fails.

> WebCore/ChangeLog:13
> +        loading resources that have been transferred to a new apge.

typo: apge->page

> WebCore/loader/DocumentLoader.cpp:792
> +    if (oldPage == m_frame->page())

There is ASSERT in other places about pages being different. Is there a reason to have 'if' here instead of ASSERT?

> WebCore/loader/DocumentLoader.h:200
> +        void transferLoadingResourcesToNewPage(Page* oldPage);

"transferLoadingResourceFromPage"? Then you can omit 'oldPage' because it'd be obvious which page it is.
I know there is "didTransferChildFrameToNewDocument(oldPage)" but here it is a bit more confusing because of 'Page' in the name of the method.
Same about name of corresponding method on FrameLoader.

> WebCore/page/Frame.cpp:747
> +        // Update resource tracking now that frame is in a different page.

"is in a different page" -> "could be in a different page" since it's before "if".

> WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1393
> +    Frame* coreFrame = core(m_webFrame.get());

if coreFrame is only used in the assert below, lets move this into assert itself.

> WebKit/mac/WebView/WebResourceLoadDelegate.h:169
> +    @param oldResource The identifier used by the old WebView to track the progress of a resource load across multiple call backs.

Would oldIdentifier be better since there is "identifier" already, and oldResource sounds like a pointer to the resource object...

[Jenn Braithwaite]

Created attachment 71306 [details]
updated patch

This patch addresses all comments by dimich in his review.

Still waiting for someone closer to Apple mac port to take a look. Not marking commit-queue-ready to ensure this does not get committed before the fix for 46915 as the test case added by this patch will trigger that bug.

[Jenn Braithwaite]

Comment on attachment 71306 [details]
updated patch

Fix for 46915 has been submitted. Flipping commit-queue? bit on.

[Jenn Braithwaite]

Comment on attachment 71306 [details]
updated patch

I will be updating this patch to remove the added Mac API.

[Jenn Braithwaite]

Created attachment 71823 [details]
updated patch

Same fix as before, but without the addition of new Mac API.

Will file a WebKit2 bug regarding the need for the former WebResourceLoadDelegate to stop tracking the resource after it has been transferred to a different page.

[Dmitry Titov]

Comment on attachment 71823 [details]
updated patch

r=me

[WebKit Commit Bot]

Comment on attachment 71823 [details]
updated patch

Rejecting patch 71823 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
/Developer/usr/bin/yacc
    /bin/sh -c /Users/abarth/git/webkit-queue/WebKitBuild/WebKit.build/Release/WebKit.build/Script-5D88EE6C11407DE800BC3ABC.sh

** BUILD FAILED **


The following build commands failed:
WebKit:
	CompileC /Users/abarth/git/webkit-queue/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WebFrameLoaderClient.o /Users/abarth/git/webkit-queue/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/4803022

[Jenn Braithwaite]

Created attachment 71933 [details]
patch to fix release version build error

Removed a local variable that was only used in an ASSERT() to fix 'unused variable' warning when building release version.

[WebKit Commit Bot]

Comment on attachment 71933 [details]
patch to fix release version build error

Clearing flags on attachment: 71933

Committed r70574: <http://trac.webkit.org/changeset/70574>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.