You need to
before you can comment on or make changes to this bug.
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).
<rdar://problem/8359858> tracks issues seen on this test case for Apple.
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?
Another option may be to make resource tracking global (not per-WebView). I'm not sure what the tradeoffs are.
(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.
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.
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?
The most interesting part here is likely making sure that Web Inspector behaves reasonably.
Note that fast/frames/iframe-reparenting-adopt-node.html is being skipped on both Mac and Windows due to this bug.
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
(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.
Created an attachment (id=70668) [details]
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.
(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.
> + 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.
Created an attachment (id=70760) [details]
Moved the trigger to update resource tracking into WebCore.
(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:
> +<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.
> + loading resources that have been transferred to a new apge.
> + 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?
> + 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.
> + // 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".
> + Frame* coreFrame = core(m_webFrame.get());
if coreFrame is only used in the assert below, lets move this into assert itself.
> + @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...
Created an attachment (id=71306) [details]
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.
(From update of attachment 71306 [details])
Fix for 46915 has been submitted. Flipping commit-queue? bit on.
(From update of attachment 71306 [details])
I will be updating this patch to remove the added Mac API.
Created an attachment (id=71823) [details]
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.
(From update of attachment 71823 [details])
(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:
/bin/sh -c /Users/abarth/git/webkit-queue/WebKitBuild/WebKit.build/Release/WebKit.build/Script-5D88EE6C11407DE800BC3ABC.sh
** BUILD FAILED **
The following build commands failed:
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
Full output: http://queues.webkit.org/results/4803022
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.
(From update of attachment 71933 [details])
Clearing flags on attachment: 71933
Committed r70574: <http://trac.webkit.org/changeset/70574>
All reviewed patches have been landed. Closing bug.