Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1859763003:
DocumentLoader should commit the load before creating a DocumentWriter (Closed)
|
Created:
4 years, 8 months ago by meacer Modified:
4 years, 8 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags.
This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument.
The order of callbacks in FrameLoader are now:
1. didInstallNewDocument
2. receivedFirstData
3. didBeginDocument
BUG=232410
Committed: https://crrev.com/3704be79c37e8fb0095c672e138241aa019f9add
Cr-Commit-Position: refs/heads/master@{#388825}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Move receivedFirstData to before didBeginDocument #Patch Set 4 : Static #Patch Set 5 : Add tests #Patch Set 6 : Fix baseURL #Patch Set 7 : Fix test #
Total comments: 2
Patch Set 8 : Move didBeginDocument #Patch Set 9 : Fix layout tests #Patch Set 10 : Rebase #
Messages
Total messages: 36 (13 generated)
Description was changed from ========== DidCommitProvisionalLoad should happen before DidCreateNewDocument. BUG= ========== to ========== DidCommitProvisionalLoad should happen before DidCreateNewDocument. BUG=232410 ==========
meacer@chromium.org changed reviewers: + jochen@chromium.org
Jochen: This is a follow up to https://codereview.chromium.org/1835753002/. I'm still looking at the failing tests so this is not fully ready for review, but if you could take an early look it would be great.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Sorry, for some reason, Rietveld decided to drop me from https://codereview.chromium.org/1835753002/ so I never saw the followup replies. Why does moving this call from ensureWriter to createWriterFor() fix the timing issue? With the exception of the archive loading path, it looks like ensureWriter() is called by commitData(). And from what I can see, we always call commitIfReady() before we we call commitData().
On 2016/04/19 00:40:19, dcheng wrote: > Sorry, for some reason, Rietveld decided to drop me from > https://codereview.chromium.org/1835753002/ so I never saw the followup replies. > > Why does moving this call from ensureWriter to createWriterFor() fix the timing > issue? With the exception of the archive loading path, it looks like > ensureWriter() is called by commitData(). And from what I can see, we always > call commitIfReady() before we we call commitData(). The difference is in the ordering of DidCreateNewDocument and DidCommitProvisionalLoad. frameLoader()->receivedFirstData() triggers a DidCommitProvisionalLoad, and frameLoader()->didBeginDocument() triggers a DidCreateNewDocument. Previously, document loading would read content settings between DidCreateNewDocument and DidCommitProvisionalLoad and see stale values since content settings cache is updated in ContentSettingsObserver::DidCommitProvisionalLoad. With this patch DidCommitProvisionalLoad comes first, content settings cache is updated and the rest of document loading uses the updated settings.
On 2016/04/19 at 00:55:30, meacer wrote: > On 2016/04/19 00:40:19, dcheng wrote: > > Sorry, for some reason, Rietveld decided to drop me from > > https://codereview.chromium.org/1835753002/ so I never saw the followup replies. > > > > Why does moving this call from ensureWriter to createWriterFor() fix the timing > > issue? With the exception of the archive loading path, it looks like > > ensureWriter() is called by commitData(). And from what I can see, we always > > call commitIfReady() before we we call commitData(). > > The difference is in the ordering of DidCreateNewDocument and DidCommitProvisionalLoad. > frameLoader()->receivedFirstData() triggers a DidCommitProvisionalLoad, and frameLoader()->didBeginDocument() triggers a DidCreateNewDocument. Doh, I see. FrameLoaderClient::dispatchDidCommitLoad() doesn't actually get dispatched by FrameLoader::commitProvisionalLoad(). That's not confusing at all… > > Previously, document loading would read content settings between DidCreateNewDocument and DidCommitProvisionalLoad and see stale values since content settings cache is updated in ContentSettingsObserver::DidCommitProvisionalLoad. With this patch DidCommitProvisionalLoad comes first, content settings cache is updated and the rest of document loading uses the updated settings. Instead of moving this into createWriterFor(), which is also used for javascript: URLs, maybe we can just move the block of code that calls receivedFirstData() higher in ensureWriter()?
On 2016/04/19 01:09:49, dcheng wrote: > On 2016/04/19 at 00:55:30, meacer wrote: > > On 2016/04/19 00:40:19, dcheng wrote: > > > Sorry, for some reason, Rietveld decided to drop me from > > > https://codereview.chromium.org/1835753002/ so I never saw the followup > replies. > > > > > > Why does moving this call from ensureWriter to createWriterFor() fix the > timing > > > issue? With the exception of the archive loading path, it looks like > > > ensureWriter() is called by commitData(). And from what I can see, we always > > > call commitIfReady() before we we call commitData(). > > > > The difference is in the ordering of DidCreateNewDocument and > DidCommitProvisionalLoad. > > frameLoader()->receivedFirstData() triggers a DidCommitProvisionalLoad, and > frameLoader()->didBeginDocument() triggers a DidCreateNewDocument. > > Doh, I see. FrameLoaderClient::dispatchDidCommitLoad() doesn't actually get > dispatched by FrameLoader::commitProvisionalLoad(). That's not confusing at all… > > > > > Previously, document loading would read content settings between > DidCreateNewDocument and DidCommitProvisionalLoad and see stale values since > content settings cache is updated in > ContentSettingsObserver::DidCommitProvisionalLoad. With this patch > DidCommitProvisionalLoad comes first, content settings cache is updated and the > rest of document loading uses the updated settings. > > Instead of moving this into createWriterFor(), which is also used for > javascript: URLs, maybe we can just move the block of code that calls > receivedFirstData() higher in ensureWriter()? Right, that's what Jochen suggested in the other CL :) But we need it after the document is created and before didBeginDocument is called, otherwise it runs into all sorts of crashes (document being null, url being incorrect etc).
Description was changed from ========== DidCommitProvisionalLoad should happen before DidCreateNewDocument. BUG=232410 ========== to ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. BUG=232410 ==========
meacer@chromium.org changed reviewers: + bauerb@chromium.org
> before didBeginDocument is called Small correction: As seen in patchset #7, this part wasn't true. The load needs to be committed after Document is created (because the IPC uses the document's security origin) and before DocumentWriter is created (where HTMLParserOptions reads the script enabled state). I think this is ready for review now that all tests pass.
https://codereview.chromium.org/1859763003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/1859763003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:675: frame->loader().receivedFirstData(); this is still after didBeginDocument. Why not move it to before?
https://codereview.chromium.org/1859763003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/1859763003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:675: frame->loader().receivedFirstData(); On 2016/04/20 14:23:25, jochen wrote: > this is still after didBeginDocument. Why not move it to before? Had to divide didBeginDocument into two: CSP and saved form data need to be initialized before committing the load, so I put them in a new didInstallNewDocument method (naming?). Moved everything else to after receivedFirstData.
lgtm
On 2016/04/21 14:46:37, jochen wrote: > lgtm Thanks!
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1859763003/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1859763003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1859763003/180001
Can you update the CL description to describe the WebFrameClient callback order before your changes vs after?
Description was changed from ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. BUG=232410 ========== to ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. The order of callbacks in FrameLoader are now: 1. didInstallNewDocument 2. receivedFirstData 3. didBeginDocument BUG=232410 ==========
Description was changed from ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. The order of callbacks in FrameLoader are now: 1. didInstallNewDocument 2. receivedFirstData 3. didBeginDocument BUG=232410 ========== to ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. The order of callbacks in FrameLoader are now: 1. didInstallNewDocument 2. receivedFirstData 3. didBeginDocument BUG=232410 ==========
On 2016/04/21 17:26:48, dcheng wrote: > Can you update the CL description to describe the WebFrameClient callback order > before your changes vs after? Done, does it lgty?
On 2016/04/21 at 17:46:01, meacer wrote: > On 2016/04/21 17:26:48, dcheng wrote: > > Can you update the CL description to describe the WebFrameClient callback order > > before your changes vs after? > > Done, does it lgty? I think I'd like to see it cover how it affects callbacks on WebFrameClient, since that's the public API change that will be visible to Blink embedders.
Message was sent while issue was closed.
Description was changed from ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. The order of callbacks in FrameLoader are now: 1. didInstallNewDocument 2. receivedFirstData 3. didBeginDocument BUG=232410 ========== to ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. The order of callbacks in FrameLoader are now: 1. didInstallNewDocument 2. receivedFirstData 3. didBeginDocument BUG=232410 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
On 2016/04/21 18:38:59, commit-bot: I haz the power wrote: > Committed patchset #10 (id:180001) Dang, I thought commit bit was unchecked. Updating the description now.
Message was sent while issue was closed.
Description was changed from ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. The order of callbacks in FrameLoader are now: 1. didInstallNewDocument 2. receivedFirstData 3. didBeginDocument BUG=232410 ========== to ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. NOTE: This also changes the order of callbacks in WebFrameClient. The OLD order when loading a document was: 1. didStartProvisionalLoad 2. didCreateNewDocument 3. didCommitProvisionalLoad 4. didCreateDocumentElement The NEW order is: 1. didStartProvisionalLoad 2. didCommitProvisionalLoad 3. didCreateNewDocument 4. didCreateDocumentElement BUG=232410 ==========
Message was sent while issue was closed.
On 2016/04/21 18:43:04, Mustafa Emre Acer wrote: > On 2016/04/21 18:38:59, commit-bot: I haz the power wrote: > > Committed patchset #10 (id:180001) > > Dang, I thought commit bit was unchecked. Updating the description now. Updated.
Message was sent while issue was closed.
On 2016/04/21 at 19:22:54, meacer wrote: > On 2016/04/21 18:43:04, Mustafa Emre Acer wrote: > > On 2016/04/21 18:38:59, commit-bot: I haz the power wrote: > > > Committed patchset #10 (id:180001) > > > > Dang, I thought commit bit was unchecked. Updating the description now. > > Updated. Too late =P Ah well. Maybe stick it in the bug, hopefully that'll be more discoverable than the code review.
Message was sent while issue was closed.
On 2016/04/21 19:35:51, dcheng wrote: > On 2016/04/21 at 19:22:54, meacer wrote: > > On 2016/04/21 18:43:04, Mustafa Emre Acer wrote: > > > On 2016/04/21 18:38:59, commit-bot: I haz the power wrote: > > > > Committed patchset #10 (id:180001) > > > > > > Dang, I thought commit bit was unchecked. Updating the description now. > > > > Updated. > > Too late =P > > Ah well. Maybe stick it in the bug, hopefully that'll be more discoverable than > the code review. I can revert and reland if you want. Looks like I snipped off the first line of the CL description as well :|
Message was sent while issue was closed.
On 2016/04/21 at 19:37:20, meacer wrote: > On 2016/04/21 19:35:51, dcheng wrote: > > On 2016/04/21 at 19:22:54, meacer wrote: > > > On 2016/04/21 18:43:04, Mustafa Emre Acer wrote: > > > > On 2016/04/21 18:38:59, commit-bot: I haz the power wrote: > > > > > Committed patchset #10 (id:180001) > > > > > > > > Dang, I thought commit bit was unchecked. Updating the description now. > > > > > > Updated. > > > > Too late =P > > > > Ah well. Maybe stick it in the bug, hopefully that'll be more discoverable than > > the code review. > > I can revert and reland if you want. Looks like I snipped off the first line of the CL description as well :| I leave it up to you. =)
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/1904183002/ by meacer@chromium.org. The reason for reverting is: Bad CL description.
Message was sent while issue was closed.
Patchset #11 (id:200001) has been deleted
Message was sent while issue was closed.
On 2016/04/21 19:51:01, Mustafa Emre Acer wrote: > A revert of this CL (patchset #10 id:180001) has been created in > https://codereview.chromium.org/1904183002/ by mailto:meacer@chromium.org. > > The reason for reverting is: Bad CL description. Ok, relanding at https://codereview.chromium.org/1910153002 with a shiny new description.
Message was sent while issue was closed.
Description was changed from ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. NOTE: This also changes the order of callbacks in WebFrameClient. The OLD order when loading a document was: 1. didStartProvisionalLoad 2. didCreateNewDocument 3. didCommitProvisionalLoad 4. didCreateDocumentElement The NEW order is: 1. didStartProvisionalLoad 2. didCommitProvisionalLoad 3. didCreateNewDocument 4. didCreateDocumentElement BUG=232410 ========== to ========== The current order of events in DocumentLoader causes an HTML parser to be created via DocumentWriter::create before the load commits. This causes the HTML parser to read a stale value for scriptEnabled setting in HTMLParserOptions if the setting changes between page loads. This in turn breaks the rendering of noscript tags. This CL moves receivedFirstData call before DocumentWriter::create and FrameLoader::didBeginDocument so that the load is committed before the HTML parser is created and the correct scriptEnabled value is used by the parser. It also moves creation of Content Security Policy and setting saved form data to a new callback didInstallNewDocument. The order of callbacks in FrameLoader are now: 1. didInstallNewDocument 2. receivedFirstData 3. didBeginDocument BUG=232410 Committed: https://crrev.com/3704be79c37e8fb0095c672e138241aa019f9add Cr-Commit-Position: refs/heads/master@{#388825} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3704be79c37e8fb0095c672e138241aa019f9add Cr-Commit-Position: refs/heads/master@{#388825} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
