REGRESSION (r87756) - Document::initSecurityContext() fails to call securityOrigin().grantLoadLocalResources()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

The Document constructor calls Document::initSecurityContext(). This function is broken because it calls Document::loader() before the frame has a chance to point to this document. This was introduced by r87756. See https://bugs.webkit.org/show_bug.cgi?id=61494#c19 and onwards.

Note that both Qt and Android use AllowLocalLoadsForLocalAndSubstituteData so rely on Document::initSecurityContext().

[Darin Adler]

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.

[Darin Adler]

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.

[Jocelyn Turcotte]

Created attachment 176271 [details]
Patch

[Jocelyn Turcotte]

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.

[Jocelyn Turcotte]

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 176271 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 176271 [details]
Patch

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

[Jocelyn Turcotte]

Created attachment 176426 [details]
Patch

Fix the chromium build.

[Adam Barth]

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.

[Adam Barth]

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().

[Nate Chapin]

(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.

[Jocelyn Turcotte]

(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.

[Jocelyn Turcotte]

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.

[WebKit Review Bot]

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

[Adam Barth]

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.

[Adam Barth]

> New failing tests:
> inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

^^^ This test is flaky.  Sorry about the noise.

[Jocelyn Turcotte]

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.

[WebKit Review Bot]

Comment on attachment 176949 [details]
Patch

Clearing flags on attachment: 176949

Committed r136404: <http://trac.webkit.org/changeset/136404>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.