Bug 68711

The core of this problem is the fact that during the creation of the Document it can’t get to its own DocumentLoader. I can think of two ways of fixing this: 1) Improving the ownership model so the document doesn’t have to go through the frame's “active document loader” to get to its document loader. This is the best long term solution. 2) Passing the document loader in when creating the document. This seems like something better to pass in than the frame, in fact. Maybe there are other fixes. (2) should be relatively mechanical so it may be a good way to do things. The only concern I have is that some documents may not have a loader.

Another possibility: 3) Hack the document initialization process so that it relies on the frame loader’s active document loader being the document’s loader despite the fact that the frame does not yet have the document as its current document. This hack could be simply implemented as a boolean that gets set during initialization and then cleared once initialization is complete. While (3) makes things more like they were before r87756, it creates the risk of reintroducing the kind of problems r87756 was supposed to solve if during document initialization there is something that causes the frame loader to move on to a new document. And (3) is not a good direction long term, since it makes things even more complicated.

Created attachment 176271 [details] Patch

This basically implements (2). It didn't feel comfortable to add a reference of DocumentLoader into Document or the other way around since both are pretty heavy objects and their lifetime can't easily be encapsulated into one another.

*** Bug 63245 has been marked as a duplicate of this bug. ***

Comment on attachment 176271 [details] Patch Attachment 176271 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15014150

Comment on attachment 176271 [details] Patch Attachment 176271 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15018234

Created attachment 176426 [details] Patch Fix the chromium build.

Comment on attachment 176426 [details] Patch There's got to be a better way of doing this. Passing in both the Frame and the DocumentLoader into the constructor is redundant.

Perhaps the DocumentLoader should be in charge of calling grantLoadLocalResources() when it is using substitute data? I don't think there is a particular reason we need to call the function during initSecurityContext().

(In reply to comment #9) > (From update of attachment 176426 [details]) > There's got to be a better way of doing this. Passing in both the Frame and the DocumentLoader into the constructor is redundant. Should Document's constructor just take a DocumentLoader* then? (In reply to comment #10) > Perhaps the DocumentLoader should be in charge of calling grantLoadLocalResources() when it is using substitute data? I don't think there is a particular reason we need to call the function during initSecurityContext(). That seems reasonable. The grantLoadLocalResources() call appears to be the only place that Document knows about SubstituteData, which makes it seem particularly out of place.

(In reply to comment #11) > (In reply to comment #10) > > Perhaps the DocumentLoader should be in charge of calling grantLoadLocalResources() when it is using substitute data? I don't think there is a particular reason we need to call the function during initSecurityContext(). > > That seems reasonable. The grantLoadLocalResources() call appears to be the only place that Document knows about SubstituteData, which makes it seem particularly out of place. Thanks for the hint, that makes it much simpler.

Created attachment 176949 [details] Patch Move the check to FrameLoader::didBeginDocument. It could also be done in DocumentLoader::commitData by returning the Document from DocumentWriter::begin if you prefer.

Comment on attachment 176949 [details] Patch Attachment 176949 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15067146 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

Comment on attachment 176949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176949&action=review > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:-2935 > - QEXPECT_FAIL("", "https://bugs.webkit.org/show_bug.cgi?id=63245", Continue); Does this mean that this test is testing change? I'm not familiar with this test harness.

> New failing tests: > inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html ^^^ This test is flaky. Sorry about the noise.

Comment on attachment 176949 [details] Patch (In reply to comment #15) > Does this mean that this test is testing change? I'm not familiar with this test harness. Thanks, yes it is.

Comment on attachment 176949 [details] Patch Clearing flags on attachment: 176949 Committed r136404: <http://trac.webkit.org/changeset/136404>

All reviewed patches have been landed. Closing bug.