Bug 44713 - Resource tracking failure when trying to move a frame between documents
: Resource tracking failure when trying to move a frame between documents
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P1 Normal
Assigned To:
:
: InRadar
: 46663 47002
: 48362 48363 48364
  Show dependency treegraph
 
Reported: 2010-08-26 13:22 PST by
Modified: 2010-10-26 14:11 PST (History)


Attachments
test case (will crash) (584 bytes, text/html)
2010-08-26 13:22 PST, Alexey Proskuryakov
no flags Details
patch (36.44 KB, patch)
2010-10-13 15:03 PST, Jenn Braithwaite
jennb: review-
Review Patch | Details | Formatted Diff | Diff
updated patch (38.08 KB, patch)
2010-10-14 11:35 PST, Jenn Braithwaite
no flags Review Patch | Details | Formatted Diff | Diff
updated patch (38.46 KB, patch)
2010-10-20 10:45 PST, Jenn Braithwaite
no flags Review Patch | Details | Formatted Diff | Diff
updated patch (34.73 KB, patch)
2010-10-25 17:22 PST, Jenn Braithwaite
dimich: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch to fix release version build error (34.69 KB, patch)
2010-10-26 13:30 PST, Jenn Braithwaite
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-26 13:22:19 PST
Created an attachment (id=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).
------- Comment #1 From 2010-08-26 13:47:19 PST -------
<rdar://problem/8359858> tracks issues seen on this test case for Apple.
------- Comment #2 From 2010-09-21 10:35:43 PST -------
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?
------- Comment #3 From 2010-09-21 10:46:41 PST -------
Another option may be to make resource tracking global (not per-WebView). I'm not sure what the tradeoffs are.
------- Comment #4 From 2010-09-21 11:10:28 PST -------
(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.
------- Comment #5 From 2010-09-21 15:39:05 PST -------
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.
------- Comment #6 From 2010-09-27 16:42:38 PST -------
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?
------- Comment #7 From 2010-09-27 16:46:49 PST -------
The most interesting part here is likely making sure that Web Inspector behaves reasonably.
------- Comment #8 From 2010-09-30 09:44:46 PST -------
Note that fast/frames/iframe-reparenting-adopt-node.html is being skipped on both Mac and Windows due to this bug.
------- Comment #9 From 2010-09-30 09:48:13 PST -------
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
------- Comment #10 From 2010-09-30 09:57:19 PST -------
(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.
------- Comment #11 From 2010-10-13 15:03:49 PST -------
Created an attachment (id=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.
------- Comment #12 From 2010-10-14 09:57:43 PST -------
(From update of attachment 70668 [details])
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.
------- Comment #13 From 2010-10-14 11:35:03 PST -------
Created an attachment (id=70760) [details]
updated patch

Moved the trigger to update resource tracking into WebCore.
------- Comment #14 From 2010-10-14 14:46:49 PST -------
(From update of attachment 70760 [details])
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...
------- Comment #15 From 2010-10-20 10:45:29 PST -------
Created an attachment (id=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.
------- Comment #16 From 2010-10-25 10:00:06 PST -------
(From update of attachment 71306 [details])
Fix for 46915 has been submitted. Flipping commit-queue? bit on.
------- Comment #17 From 2010-10-25 16:09:26 PST -------
(From update of attachment 71306 [details])
I will be updating this patch to remove the added Mac API.
------- Comment #18 From 2010-10-25 17:22:21 PST -------
Created an attachment (id=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.
------- Comment #19 From 2010-10-26 12:43:25 PST -------
(From update of attachment 71823 [details])
r=me
------- Comment #20 From 2010-10-26 13:13:36 PST -------
(From update of attachment 71823 [details])
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
------- Comment #21 From 2010-10-26 13:30:53 PST -------
Created an attachment (id=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.
------- Comment #22 From 2010-10-26 14:10:59 PST -------
(From update of attachment 71933 [details])
Clearing flags on attachment: 71933

Committed r70574: <http://trac.webkit.org/changeset/70574>
------- Comment #23 From 2010-10-26 14:11:07 PST -------
All reviewed patches have been landed.  Closing bug.