Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1835753002:
Reset content settings caches during provisional load start instead of commit. (Closed)
DescriptionReset content settings caches during provisional load start instead of commit.
ContentSettingsObserver caches some content settings for performance
reasons (cached_storage_permissions_, cached_script_permissions_ and
temporarily_allowed_plugins_). These are cleared in DidCommitProvisionalLoad.
However, a frame can query JS blocking status before DidCommitProvisionalLoad,
and can see a stale setting if the setting is changed before page load. This
causes noscript tags not being rendered or rendered alongside script tags.
This CL clears the cache in DidStartProvisionalLoad instead, allowing all page
loading code to see the same content setting.
BUG=232410
Patch Set 1 #Patch Set 2 : Fix android build #Patch Set 3 : Remove same page check #Patch Set 4 : Add a browsertest #Patch Set 5 : Add another browsertest #
Total comments: 6
Patch Set 6 : Move things under DidCreateNewDocument instead of DidStartProvisionalLoad #
Total comments: 2
Messages
Total messages: 31 (4 generated)
Description was changed from ========== Reset content settings caches during provisional load start instead of commit. ContentSettingsObserver keeps a cache of blocked content settings for performance reasons. The cache is cleared in DidCommitProvisionalLoad. However, a frame can execute JS before DidCommitProvisionalLoad. This CL clears the cache in DidStartProvisionalLoad instead. BUG=232410 ========== to ========== Reset content settings caches during provisional load start instead of commit. ContentSettingsObserver caches blocked content settings for performance reasons (cached_storage_permissions_, cached_script_permissions_ and temporarily_allowed_plugins_). These are cleared in DidCommitProvisionalLoad. However, a frame can execute JS before DidCommitProvisionalLoad and can read a stale setting if the setting is changed right before page load. This CL clears the cache in DidStartProvisionalLoad instead, allowing all of the frame to see the same setting. BUG=232410 ==========
Description was changed from ========== Reset content settings caches during provisional load start instead of commit. ContentSettingsObserver caches blocked content settings for performance reasons (cached_storage_permissions_, cached_script_permissions_ and temporarily_allowed_plugins_). These are cleared in DidCommitProvisionalLoad. However, a frame can execute JS before DidCommitProvisionalLoad and can read a stale setting if the setting is changed right before page load. This CL clears the cache in DidStartProvisionalLoad instead, allowing all of the frame to see the same setting. BUG=232410 ========== to ========== Reset content settings caches during provisional load start instead of commit. ContentSettingsObserver caches some content settings for performance reasons (cached_storage_permissions_, cached_script_permissions_ and temporarily_allowed_plugins_). These are cleared in DidCommitProvisionalLoad. However, a frame can query JS blocking status before DidCommitProvisionalLoad, and can see a stale setting if the setting is changed before page load. This causes noscript tags not being rendered or rendered alongside script tags. This CL clears the cache in DidStartProvisionalLoad instead, allowing all page loading code to see the same content setting. BUG=232410 ==========
meacer@chromium.org changed reviewers: + thestig@chromium.org
thestig: Can you PTAL? Thanks.
thestig@chromium.org changed reviewers: + bauerb@chromium.org, jochen@chromium.org
jochen/bauerb are probably more qualified to review this. https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... File chrome/renderer/content_settings_observer_browsertest.cc (right): https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... chrome/renderer/content_settings_observer_browsertest.cc:346: std::string html = Can this just be const char kHtml[] ? https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... chrome/renderer/content_settings_observer_browsertest.cc:357: content_setting_rules.script_rules.clear(); Reuse |script_setting_rules| ?
Note that the things that are cleared here aren't purely caches of information that we'd otherwise have; they include things like whether we the user has allowed plugins to run "this time" (which we usually take to mean until they navigate somewhere else). https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... chrome/renderer/content_settings_observer.cc:240: void ContentSettingsObserver::DidStartProvisionalLoad() { Hm... isn't this called when we send out a network request for a new page? Then it could happen that the network request doesn't actually succeed (i.e. no navigation takes place), but we'll still have reset these.
On 2016/03/29 08:33:34, Bernhard Bauer wrote: > Note that the things that are cleared here aren't purely caches of information > that we'd otherwise have; they include things like whether we the user has > allowed plugins to run "this time" (which we usually take to mean until they > navigate somewhere else). Ah, thanks for clarifying. I'll update the CL to reflect that. > > https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... > File chrome/renderer/content_settings_observer.cc (right): > > https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... > chrome/renderer/content_settings_observer.cc:240: void > ContentSettingsObserver::DidStartProvisionalLoad() { > Hm... isn't this called when we send out a network request for a new page? Then > it could happen that the network request doesn't actually succeed (i.e. no > navigation takes place), but we'll still have reset these.
https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... chrome/renderer/content_settings_observer.cc:240: void ContentSettingsObserver::DidStartProvisionalLoad() { On 2016/03/29 08:33:33, Bernhard Bauer wrote: > Hm... isn't this called when we send out a network request for a new page? Then > it could happen that the network request doesn't actually succeed (i.e. no > navigation takes place), but we'll still have reset these. It sounds like this would only affect temporarily allowed plugins though, since blocked content settings will just be re-read. The next best point to do this is DidCreateNewDocument, but there is at least one call to canExecuteScript between DidStartProvisionalLoad and DidCreateNewDocument so it's probably best to keep it in DidStartProvisionalLoad. I think clearing the plugin exception when we hit a network error is an edge case, but if you think we shouldn't do that, I can move |temporarily_allowed_plugins_.clear()| line to DidCreateNewDocument.
before the provisional load committed, the old URL is still active, so if we reget the content settings, we should get the same result, no?
On 2016/03/30 16:31:34, jochen wrote: > before the provisional load committed, the old URL is still active, so if we > reget the content settings, we should get the same result, no? Okay, makes sense. I moved the reset cache & plugin blockage code to DidCreateNewDocument. PTAL.
https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... File chrome/renderer/content_settings_observer_browsertest.cc (right): https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... chrome/renderer/content_settings_observer_browsertest.cc:346: std::string html = On 2016/03/29 00:13:45, Lei Zhang wrote: > Can this just be const char kHtml[] ? Done. https://codereview.chromium.org/1835753002/diff/80001/chrome/renderer/content... chrome/renderer/content_settings_observer_browsertest.cc:357: content_setting_rules.script_rules.clear(); On 2016/03/29 00:13:45, Lei Zhang wrote: > Reuse |script_setting_rules| ? Done.
LGTM!
I don't think that's the right solution. I'm not quite sure what happens here. Can you post stack traces to the allowScript calls that happen after didCommitProvisionalLoad and once return allow script and later don't allow script? https://codereview.chromium.org/1835753002/diff/90001/chrome/renderer/content... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1835753002/diff/90001/chrome/renderer/content... chrome/renderer/content_settings_observer.cc:227: void ContentSettingsObserver::DidCreateNewDocument() { didCreateNewDocument is also invoked when somebody invokes document.open()
On 2016/03/31 15:26:20, jochen wrote: > I don't think that's the right solution. To be clear, there is currently one more call to allowScript after FrameWillClose and before DidCreateNewDocument when the frame loads another URL. That allowScript check is done using the new URL's origin, even though the new document isn't yet loaded. > > I'm not quite sure what happens here. Can you post stack traces to the > allowScript calls that happen after didCommitProvisionalLoad and once return > allow script and later don't allow script? The stack is the same for allow/block script cases, both with and without the patch (save for the parameter addresses): http://pastebin.com/raw/PDkZ1eAC > > https://codereview.chromium.org/1835753002/diff/90001/chrome/renderer/content... > File chrome/renderer/content_settings_observer.cc (right): > > https://codereview.chromium.org/1835753002/diff/90001/chrome/renderer/content... > chrome/renderer/content_settings_observer.cc:227: void > ContentSettingsObserver::DidCreateNewDocument() { > didCreateNewDocument is also invoked when somebody invokes document.open()
https://codereview.chromium.org/1835753002/diff/90001/chrome/renderer/content... File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1835753002/diff/90001/chrome/renderer/content... chrome/renderer/content_settings_observer.cc:227: void ContentSettingsObserver::DidCreateNewDocument() { On 2016/03/31 15:26:19, jochen wrote: > didCreateNewDocument is also invoked when somebody invokes document.open() Did you mean DidCreateDocumentElement? That's the one I'm seeing with document.open(). window.open() does invoke didCreateNewDocument but it's for the newly opened url.
On 2016/04/01 at 19:06:44, meacer wrote: > On 2016/03/31 15:26:20, jochen wrote: > > I don't think that's the right solution. > > To be clear, there is currently one more call to allowScript after FrameWillClose and before DidCreateNewDocument when the frame loads another URL. That allowScript check is done using the new URL's origin, even though the new document isn't yet loaded. > > > > > I'm not quite sure what happens here. Can you post stack traces to the > > allowScript calls that happen after didCommitProvisionalLoad and once return > > allow script and later don't allow script? > > The stack is the same for allow/block script cases, both with and without the patch (save for the parameter addresses): > > http://pastebin.com/raw/PDkZ1eAC this is the call that returns a different value with and without your patch?
On 2016/04/01 20:08:47, jochen wrote: > On 2016/04/01 at 19:06:44, meacer wrote: > > On 2016/03/31 15:26:20, jochen wrote: > > > I don't think that's the right solution. > > > > To be clear, there is currently one more call to allowScript after > FrameWillClose and before DidCreateNewDocument when the frame loads another URL. > That allowScript check is done using the new URL's origin, even though the new > document isn't yet loaded. > > > > > > > > I'm not quite sure what happens here. Can you post stack traces to the > > > allowScript calls that happen after didCommitProvisionalLoad and once return > > > allow script and later don't allow script? > > > > The stack is the same for allow/block script cases, both with and without the > patch (save for the parameter addresses): > > > > http://pastebin.com/raw/PDkZ1eAC > > this is the call that returns a different value with and without your patch? Yes, but it only returns a different a value when the content settings change. Without the patch, it returns stale content settings value when parsing the document. E.g. if user blocks JS and reloads the page, allowScript will still return true while the document is being parsed, causing noscript tags to not be displayed. After provisional load commits, allowScript starts returning false (JS is now blocked), but it's already too late. With the patch, parsing sees the most recent content setting. Is that what you meant?
On 2016/04/01 at 20:16:05, meacer wrote: > On 2016/04/01 20:08:47, jochen wrote: > > On 2016/04/01 at 19:06:44, meacer wrote: > > > On 2016/03/31 15:26:20, jochen wrote: > > > > I don't think that's the right solution. > > > > > > To be clear, there is currently one more call to allowScript after > > FrameWillClose and before DidCreateNewDocument when the frame loads another URL. > > That allowScript check is done using the new URL's origin, even though the new > > document isn't yet loaded. > > > > > > > > > > > I'm not quite sure what happens here. Can you post stack traces to the > > > > allowScript calls that happen after didCommitProvisionalLoad and once return > > > > allow script and later don't allow script? > > > > > > The stack is the same for allow/block script cases, both with and without the > > patch (save for the parameter addresses): > > > > > > http://pastebin.com/raw/PDkZ1eAC > > > > this is the call that returns a different value with and without your patch? > > Yes, but it only returns a different a value when the content settings change. Without the patch, it returns stale content settings value when parsing the document. > E.g. if user blocks JS and reloads the page, allowScript will still return true while the document is being parsed, causing noscript tags to not be displayed. After provisional load commits, allowScript starts returning false (JS is now blocked), but it's already too late. > > With the patch, parsing sees the most recent content setting. Is that what you meant? But didCommitProvisionalLoad comes first, your patch delays clearing of the settings
On 2016/04/01 20:21:03, jochen wrote: > On 2016/04/01 at 20:16:05, meacer wrote: > > On 2016/04/01 20:08:47, jochen wrote: > > > On 2016/04/01 at 19:06:44, meacer wrote: > > > > On 2016/03/31 15:26:20, jochen wrote: > > > > > I don't think that's the right solution. > > > > > > > > To be clear, there is currently one more call to allowScript after > > > FrameWillClose and before DidCreateNewDocument when the frame loads another > URL. > > > That allowScript check is done using the new URL's origin, even though the > new > > > document isn't yet loaded. > > > > > > > > > > > > > > I'm not quite sure what happens here. Can you post stack traces to the > > > > > allowScript calls that happen after didCommitProvisionalLoad and once > return > > > > > allow script and later don't allow script? > > > > > > > > The stack is the same for allow/block script cases, both with and without > the > > > patch (save for the parameter addresses): > > > > > > > > http://pastebin.com/raw/PDkZ1eAC > > > > > > this is the call that returns a different value with and without your patch? > > > > Yes, but it only returns a different a value when the content settings change. > Without the patch, it returns stale content settings value when parsing the > document. > > E.g. if user blocks JS and reloads the page, allowScript will still return > true while the document is being parsed, causing noscript tags to not be > displayed. After provisional load commits, allowScript starts returning false > (JS is now blocked), but it's already too late. > > > > With the patch, parsing sees the most recent content setting. Is that what you > meant? > > But didCommitProvisionalLoad comes first, your patch delays clearing of the > settings The order I'm seeing is when loading a new URL is: DidStartProvisionalLoad FrameWillClose DidClearWindowObject DidCreateNewDocument DidCommitProvisionalLoad
On 2016/04/01 at 20:27:10, meacer wrote: > On 2016/04/01 20:21:03, jochen wrote: > > On 2016/04/01 at 20:16:05, meacer wrote: > > > On 2016/04/01 20:08:47, jochen wrote: > > > > On 2016/04/01 at 19:06:44, meacer wrote: > > > > > On 2016/03/31 15:26:20, jochen wrote: > > > > > > I don't think that's the right solution. > > > > > > > > > > To be clear, there is currently one more call to allowScript after > > > > FrameWillClose and before DidCreateNewDocument when the frame loads another > > URL. > > > > That allowScript check is done using the new URL's origin, even though the > > new > > > > document isn't yet loaded. > > > > > > > > > > > > > > > > > I'm not quite sure what happens here. Can you post stack traces to the > > > > > > allowScript calls that happen after didCommitProvisionalLoad and once > > return > > > > > > allow script and later don't allow script? > > > > > > > > > > The stack is the same for allow/block script cases, both with and without > > the > > > > patch (save for the parameter addresses): > > > > > > > > > > http://pastebin.com/raw/PDkZ1eAC > > > > > > > > this is the call that returns a different value with and without your patch? > > > > > > Yes, but it only returns a different a value when the content settings change. > > Without the patch, it returns stale content settings value when parsing the > > document. > > > E.g. if user blocks JS and reloads the page, allowScript will still return > > true while the document is being parsed, causing noscript tags to not be > > displayed. After provisional load commits, allowScript starts returning false > > (JS is now blocked), but it's already too late. > > > > > > With the patch, parsing sees the most recent content setting. Is that what you > > meant? > > > > But didCommitProvisionalLoad comes first, your patch delays clearing of the > > settings > > The order I'm seeing is when loading a new URL is: > > DidStartProvisionalLoad > FrameWillClose > DidClearWindowObject > DidCreateNewDocument > DidCommitProvisionalLoad That order looks very wrong. I'd like japhet@ or dcheng@ chime in on this
dcheng, japhet: Can you please comment on the ordering of RenderFrameObserver events when a new url is loaded?
On 2016/04/01 at 21:03:37, meacer wrote: > dcheng, japhet: Can you please comment on the ordering of RenderFrameObserver events when a new url is loaded? Hmm... from what I can tell: For a "regular" navigation, it should be: FrameWillClose DidCreateNewDocument DidCommitProvisionalLoad DidClearWindowObject How are you seeing it occur in that order?
On 2016/04/01 22:26:58, dcheng wrote: > On 2016/04/01 at 21:03:37, meacer wrote: > > dcheng, japhet: Can you please comment on the ordering of RenderFrameObserver > events when a new url is loaded? > > Hmm... from what I can tell: > > For a "regular" navigation, it should be: > FrameWillClose > DidCreateNewDocument > DidCommitProvisionalLoad > DidClearWindowObject > > How are you seeing it occur in that order? I see two slightly different cases: - From new tab page to localhost:7654, it's the same order as yours. - From localhost:7654 to localhost:6543, I see two |DidClearWindowObject|s, one before DidCreateNewDocument and one after DidCommitProvisionalLoad. So there is an extra DidClearWindowObject call here, before DidCreateNewDocument. But in any case DidCreateNewDocument comes before DidCommitProvisionalLoad.
On 2016/04/01 at 22:37:25, meacer wrote: > On 2016/04/01 22:26:58, dcheng wrote: > > On 2016/04/01 at 21:03:37, meacer wrote: > > > dcheng, japhet: Can you please comment on the ordering of RenderFrameObserver > > events when a new url is loaded? > > > > Hmm... from what I can tell: > > > > For a "regular" navigation, it should be: > > FrameWillClose > > DidCreateNewDocument > > DidCommitProvisionalLoad > > DidClearWindowObject > > > > How are you seeing it occur in that order? > > I see two slightly different cases: > - From new tab page to localhost:7654, it's the same order as yours. > - From localhost:7654 to localhost:6543, I see two |DidClearWindowObject|s, one before DidCreateNewDocument and one after DidCommitProvisionalLoad. So there is an extra DidClearWindowObject call here, before DidCreateNewDocument. > > But in any case DidCreateNewDocument comes before DidCommitProvisionalLoad. How is it possible that we do anything in the new frame before the load has committed? We just don't know what origin to use before that event
ah, I see now that DocumentLoader::ensureWriter first creates the writer and then calls FrameLoader::receivedFirstData. I think the right fix is to first call receivedFirstData here.
On 2016/04/03 12:50:20, jochen wrote: > ah, I see now that DocumentLoader::ensureWriter first creates the writer and > then calls FrameLoader::receivedFirstData. > > I think the right fix is to first call receivedFirstData here. Just so I'm understanding correctly: I *think* there is an existing bug here, in that the code is trying to make security decisions for two origins using the same object: Both the current URL and the new URL being loaded share the same |cached_script_permissions_| until the document for the new URL is committed. Seems like ContentSettingsObserver should be able to differentiate between these two. Is my understanding correct? If not, I'm not sure I understand the problem here (particularly why we need to call receivedFirstData upon a content settings change? Should this class really be responsible for that?)
On 2016/04/04 at 06:36:28, meacer wrote: > On 2016/04/03 12:50:20, jochen wrote: > > ah, I see now that DocumentLoader::ensureWriter first creates the writer and > > then calls FrameLoader::receivedFirstData. > > > > I think the right fix is to first call receivedFirstData here. > > Just so I'm understanding correctly: > I *think* there is an existing bug here, in that the code is trying to make security decisions for two origins using the same object: Both the current URL and the new URL being loaded share the same |cached_script_permissions_| until the document for the new URL is committed. Seems like ContentSettingsObserver should be able to differentiate between these two. Is my understanding correct? If not, I'm not sure I understand the problem here (particularly why we need to call receivedFirstData upon a content settings change? Should this class really be responsible for that?) The call should be made in DocumentLoader::ensureWriter, not in the content settings observer. for the observer, there should only every be one URL, and it should change at the moment didCommitProvisionalLoad is called
On 2016/04/04 12:07:50, jochen wrote: > On 2016/04/04 at 06:36:28, meacer wrote: > > On 2016/04/03 12:50:20, jochen wrote: > > > ah, I see now that DocumentLoader::ensureWriter first creates the writer and > > > then calls FrameLoader::receivedFirstData. > > > > > > I think the right fix is to first call receivedFirstData here. > > > > Just so I'm understanding correctly: > > I *think* there is an existing bug here, in that the code is trying to make > security decisions for two origins using the same object: Both the current URL > and the new URL being loaded share the same |cached_script_permissions_| until > the document for the new URL is committed. Seems like ContentSettingsObserver > should be able to differentiate between these two. Is my understanding correct? > If not, I'm not sure I understand the problem here (particularly why we need to > call receivedFirstData upon a content settings change? Should this class really > be responsible for that?) > > The call should be made in DocumentLoader::ensureWriter, not in the content > settings observer. DocumentLoader::ensureWriter already calls FrameLoader::receivedFirstData, do you mean we should do it earlier in the function? The earliest location that works is right after creating the writer (createWriterFor), and that's only 3 statements earlier. Anywhere else causes the renderer to terminate on bad IPC. I feel like I still don't quite understand the problem with or without the patch :/ Is it that the order of events look wrong? (e.g. DidCreateNewDocument before DidCommitProvisionalLoad) > > for the observer, there should only every be one URL, and it should change at > the moment didCommitProvisionalLoad is called I'm not sure if this is correct. We still need to know script blocking status while loading and parsing the document which as I understand happens before didCommitProvisionalLoad. Hence my previous comment about cached_script_permissions_ being keyed on WebFrame rather than URL/origin: While parsing the new document we end up calling allowScript which returns the content setting for the committed page, not the page being loaded which is buggy.
On 2016/04/04 17:26:50, Mustafa Emre Acer wrote: > On 2016/04/04 12:07:50, jochen wrote: > > On 2016/04/04 at 06:36:28, meacer wrote: > > > On 2016/04/03 12:50:20, jochen wrote: > > > > ah, I see now that DocumentLoader::ensureWriter first creates the writer > and > > > > then calls FrameLoader::receivedFirstData. > > > > > > > > I think the right fix is to first call receivedFirstData here. > > > > > > Just so I'm understanding correctly: > > > I *think* there is an existing bug here, in that the code is trying to make > > security decisions for two origins using the same object: Both the current URL > > and the new URL being loaded share the same |cached_script_permissions_| until > > the document for the new URL is committed. Seems like ContentSettingsObserver > > should be able to differentiate between these two. Is my understanding > correct? > > If not, I'm not sure I understand the problem here (particularly why we need > to > > call receivedFirstData upon a content settings change? Should this class > really > > be responsible for that?) > > > > The call should be made in DocumentLoader::ensureWriter, not in the content > > settings observer. > > DocumentLoader::ensureWriter already calls FrameLoader::receivedFirstData, do > you mean we should do it earlier in the function? The earliest location that > works is right after creating the writer (createWriterFor), and that's only 3 > statements earlier. Anywhere else causes the renderer to terminate on bad IPC. > > I feel like I still don't quite understand the problem with or without the patch > :/ Is it that the order of events look wrong? (e.g. DidCreateNewDocument before > DidCommitProvisionalLoad) > > > > > for the observer, there should only every be one URL, and it should change at > > the moment didCommitProvisionalLoad is called > > I'm not sure if this is correct. We still need to know script blocking status > while loading and parsing the document which as I understand happens before > didCommitProvisionalLoad. Hence my previous comment about > cached_script_permissions_ being keyed on WebFrame rather than URL/origin: While > parsing the new document we end up calling allowScript which returns the content > setting for the committed page, not the page being loaded which is buggy. Got back to this after 2 weeks. Jochen and I discussed this in person and he suggests that we should change the ordering of events to DidCommitProvisionalLoad DidCreateNewDocument Simply moving frameLoader()->receivedFirstData above in DocumentLoader::createWriterFor in order to do this doesn't wor: we hit navigation checks for the url and the security origin in RenderFrameHostImpl::CanCommitURL. I'm looking to move things around so that we have the correct URL before hitting that check.
Closing this in favor of https://codereview.chromium.org/1859763003/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
