Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2099563002:
WebUI: DisallowJavascript only on Refresh and non-same-page navigations (Closed)
DescriptionWebUI: DisallowJavascript only on Refresh and non-same-page navigations
Previously, WebUI handlers were being disabled via DisallowJavascript
when the user clicked the browser's Back and Forward buttons, even for
in-page navigations (fragments or window.history.push).
That led to crashes and unexpected behavior.
BUG=621885
R=creis@chromium.org, dbeam@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/e525223a1c71de1106cf8e23c54d239d298d9982
Patch Set 1 #Patch Set 2 : small fixes #
Total comments: 10
Patch Set 3 : add test #
Total comments: 6
Patch Set 4 : add a reload portion to test #Patch Set 5 : name fix #
Total comments: 5
Patch Set 6 : Use a test handler on chrome://terms instead #
Total comments: 2
Patch Set 7 : fix style #
Messages
Total messages: 32 (9 generated)
tommycli@chromium.org changed reviewers: + creis@chromium.org
creis: PTAL, thanks! - this is the in-page navigation fix for WebUI handlers we discussed earlier this week.
On 2016/06/24 00:24:02, tommycli wrote: > creis: PTAL, thanks! - this is the in-page navigation fix for WebUI handlers we > discussed earlier this week. Great-- I'll try to take a look later today (as things are a bit hectic this morning).
On 2016/06/24 16:10:46, Charlie Reis wrote: > On 2016/06/24 00:24:02, tommycli wrote: > > creis: PTAL, thanks! - this is the in-page navigation fix for WebUI handlers > we > > discussed earlier this week. > > Great-- I'll try to take a look later today (as things are a bit hectic this > morning). No problem - thanks!
Great! Can you add a test that does a back after an in-page WebUI navigation and ensures that Javascript is still allowed? https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... File content/browser/webui/web_ui_impl.cc (right): https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:24: #include "content/public/browser/web_ui_controller.h" IWYU nit: web_contents_observer.h https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:39: if (!navigation_handle->IsInMainFrame() || // Only disallow Javascript on cross-document navigations in the main frame. https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:47: WebUIImpl* parent_; web_ui_, perhaps? (Parent is a confusing term, since it might refer to frames or other hierarchical WebContents things like <webview>.) https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:117: GetContentClient()->browser()->LogWebUIUrl(site_url); Ooh, I wonder if we can get rid of RenderViewReused entirely by doing this logging in DidFinishNavigation... That would be really nice. We probably shouldn't do it in this CL, though, to avoid the risk of affecting logs and possibly needing to revert. Does it sound worth trying in a followup CL? https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.h:126: // Used to allow WebUI to be notified of WebContents navigations. // Notifies this WebUI about notifications in the main frame.
creis: Thanks. I added a test also for the inpage navigations. https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... File content/browser/webui/web_ui_impl.cc (right): https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:24: #include "content/public/browser/web_ui_controller.h" On 2016/06/24 23:15:00, Charlie Reis wrote: > IWYU nit: web_contents_observer.h Done. https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:39: if (!navigation_handle->IsInMainFrame() || On 2016/06/24 23:15:00, Charlie Reis wrote: > // Only disallow Javascript on cross-document navigations in the main frame. Done. https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:47: WebUIImpl* parent_; On 2016/06/24 23:15:00, Charlie Reis wrote: > web_ui_, perhaps? (Parent is a confusing term, since it might refer to frames > or other hierarchical WebContents things like <webview>.) Done. https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:117: GetContentClient()->browser()->LogWebUIUrl(site_url); On 2016/06/24 23:15:01, Charlie Reis wrote: > Ooh, I wonder if we can get rid of RenderViewReused entirely by doing this > logging in DidFinishNavigation... That would be really nice. > > We probably shouldn't do it in this CL, though, to avoid the risk of affecting > logs and possibly needing to revert. Does it sound worth trying in a followup > CL? Yeah I think that would be eminently doable. But yes, like you said, in a separate CL. https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... File content/browser/webui/web_ui_impl.h (right): https://codereview.chromium.org/2099563002/diff/20001/content/browser/webui/w... content/browser/webui/web_ui_impl.h:126: // Used to allow WebUI to be notified of WebContents navigations. On 2016/06/24 23:15:01, Charlie Reis wrote: > // Notifies this WebUI about notifications in the main frame. Done.
Thanks for the quick test! One note about WaitForLoadStop and a few nits. Otherwise LGTM. https://codereview.chromium.org/2099563002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:69: for (content::WebUIMessageHandler* handler : *handlers) { nit: No braces needed. Same below. https://codereview.chromium.org/2099563002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:77: chrome::GoBack(browser(), CURRENT_TAB); You might need to use WaitForLoadStop to avoid flakiness. (Did the test fail without the fix?) https://codereview.chromium.org/2099563002/diff/40001/content/browser/webui/w... File content/browser/webui/web_ui_impl.cc (right): https://codereview.chromium.org/2099563002/diff/40001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:34: MainFrameNavigationObserver(WebUIImpl* parent, WebContents* contents) nit: s/parent/web_ui/
creis: thanks! I also added a Reload portion to the test. I think it's good to test that Reload still disallows some of the handlers. However, some of the handlers are re-enabled by JavaScript, so the test condition is pretty weak. PTAL -I'd appreciate a second opinion on that part too. Thanks! https://codereview.chromium.org/2099563002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:69: for (content::WebUIMessageHandler* handler : *handlers) { On 2016/06/25 00:02:19, Charlie Reis wrote: > nit: No braces needed. Same below. Done. https://codereview.chromium.org/2099563002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:77: chrome::GoBack(browser(), CURRENT_TAB); On 2016/06/25 00:02:19, Charlie Reis wrote: > You might need to use WaitForLoadStop to avoid flakiness. > > (Did the test fail without the fix?) I didn't detect any flakiness on my machine (the test failed without the fix), but I might just have a fast machine. I added the wait. https://codereview.chromium.org/2099563002/diff/40001/content/browser/webui/w... File content/browser/webui/web_ui_impl.cc (right): https://codereview.chromium.org/2099563002/diff/40001/content/browser/webui/w... content/browser/webui/web_ui_impl.cc:34: MainFrameNavigationObserver(WebUIImpl* parent, WebContents* contents) On 2016/06/25 00:02:19, Charlie Reis wrote: > nit: s/parent/web_ui/ Done.
https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:85: // Verify that after a reload, at least some of the handlers are disallowed. I'm not sure I understand this. Wouldn't all of the handlers come back if you waited long enough? Or do some of them go away permanently when you reload? (Why would that be?)
https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:85: // Verify that after a reload, at least some of the handlers are disallowed. On 2016/06/25 00:17:02, Charlie Reis wrote: > I'm not sure I understand this. Wouldn't all of the handlers come back if you > waited long enough? Or do some of them go away permanently when you reload? > (Why would that be?) Some of the handlers only are used on subpages. They are not 'activated' until they receive their first chrome.send message they need to handle. Since we are loading the main page only, some of these subpage handlers are never activated.
https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:85: // Verify that after a reload, at least some of the handlers are disallowed. On 2016/06/25 00:21:51, tommycli wrote: > On 2016/06/25 00:17:02, Charlie Reis wrote: > > I'm not sure I understand this. Wouldn't all of the handlers come back if you > > waited long enough? Or do some of them go away permanently when you reload? > > (Why would that be?) > > Some of the handlers only are used on subpages. They are not 'activated' until > they receive their first chrome.send message they need to handle. > > Since we are loading the main page only, some of these subpage handlers are > never activated. I'm trying to test that on Reload, the handlers are all disabled. I'm totally open to a better way of testing this, since this way is confusing and kind of lame.
Sorry for the delay-- two possible ideas below. https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:85: // Verify that after a reload, at least some of the handlers are disallowed. On 2016/06/25 00:22:50, tommycli wrote: > On 2016/06/25 00:21:51, tommycli wrote: > > On 2016/06/25 00:17:02, Charlie Reis wrote: > > > I'm not sure I understand this. Wouldn't all of the handlers come back if > you > > > waited long enough? Or do some of them go away permanently when you reload? > > > > (Why would that be?) > > > > Some of the handlers only are used on subpages. They are not 'activated' until > > they receive their first chrome.send message they need to handle. > > > > Since we are loading the main page only, some of these subpage handlers are > > never activated. Hmm. Would the list of enabled handlers match the list after line 65, before we loop through and call AllowJavascriptForTesting on them on line 67? Maybe we could grab the ones that are disabled and ensure they're disabled again after the reload? (I'm not sure if it's that simple.) > > I'm trying to test that on Reload, the handlers are all disabled. > > I'm totally open to a better way of testing this, since this way is confusing > and kind of lame. Another option might be adding a test message handler that can listen for whether disallow and allow were called on it.
creis: thanks! The revised test is looking much better to me. https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_ui_browsertest.cc:85: // Verify that after a reload, at least some of the handlers are disallowed. On 2016/06/27 21:31:31, Charlie Reis wrote: > On 2016/06/25 00:22:50, tommycli wrote: > > On 2016/06/25 00:21:51, tommycli wrote: > > > On 2016/06/25 00:17:02, Charlie Reis wrote: > > > > I'm not sure I understand this. Wouldn't all of the handlers come back if > > you > > > > waited long enough? Or do some of them go away permanently when you > reload? > > > > > > (Why would that be?) > > > > > > Some of the handlers only are used on subpages. They are not 'activated' > until > > > they receive their first chrome.send message they need to handle. > > > > > > Since we are loading the main page only, some of these subpage handlers are > > > never activated. > > Hmm. Would the list of enabled handlers match the list after line 65, before we > loop through and call AllowJavascriptForTesting on them on line 67? Maybe we > could grab the ones that are disabled and ensure they're disabled again after > the reload? (I'm not sure if it's that simple.) > > > > > I'm trying to test that on Reload, the handlers are all disabled. > > > > I'm totally open to a better way of testing this, since this way is confusing > > and kind of lame. > > Another option might be adding a test message handler that can listen for > whether disallow and allow were called on it. Ah yes. Using the test handler was the best suggestion IMO. Since it was a test handler I was also able to use chrome://terms instead of chrome://md-settings (much faster to run browsertest), and the test retains the power it had. I moved this test to webui_browsertest.cc btw, and renamed the tests there to WebUIImplBrowserTest, since there was already a WebUIBrowserTest class and the name collided.
Ah, much better. New test LGTM. https://codereview.chromium.org/2099563002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/webui_browsertest.cc:25: void RegisterMessages() override{}; Style nit: Space after override (unless git cl format disagrees).
creis: awesome, thanks for the testing help! https://codereview.chromium.org/2099563002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/webui_browsertest.cc (right): https://codereview.chromium.org/2099563002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/webui_browsertest.cc:25: void RegisterMessages() override{}; On 2016/06/27 22:18:19, Charlie Reis wrote: > Style nit: Space after override (unless git cl format disagrees). Done. Ah git cl format got confused by the extra semicolon. Now it looks good.
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2099563002/#ps120001 (title: "fix style")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tommycli@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: PTAL need OWNER stamp chrome/browser/ui/webui/webui_browsertest.cc, thanks!
lgtm
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Description was changed from ========== WebUI: DisallowJavascript only on Refresh and non-same-page navigations Previously, WebUI handlers were being disabled via DisallowJavascript when the user clicked the browser's Back and Forward buttons, even for in-page navigations (fragments or window.history.push). That led to crashes and unexpected behavior. BUG=621885 ========== to ========== WebUI: DisallowJavascript only on Refresh and non-same-page navigations Previously, WebUI handlers were being disabled via DisallowJavascript when the user clicked the browser's Back and Forward buttons, even for in-page navigations (fragments or window.history.push). That led to crashes and unexpected behavior. BUG=621885 R=creis@chromium.org, dbeam@chromium.org Committed: https://crrev.com/e525223a1c71de1106cf8e23c54d239d298d9982 Cr-Commit-Position: refs/heads/master@{#402507} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e525223a1c71de1106cf8e23c54d239d298d9982 Cr-Commit-Position: refs/heads/master@{#402507}
Message was sent while issue was closed.
Description was changed from ========== WebUI: DisallowJavascript only on Refresh and non-same-page navigations Previously, WebUI handlers were being disabled via DisallowJavascript when the user clicked the browser's Back and Forward buttons, even for in-page navigations (fragments or window.history.push). That led to crashes and unexpected behavior. BUG=621885 R=creis@chromium.org, dbeam@chromium.org Committed: https://crrev.com/e525223a1c71de1106cf8e23c54d239d298d9982 Cr-Commit-Position: refs/heads/master@{#402507} ========== to ========== WebUI: DisallowJavascript only on Refresh and non-same-page navigations Previously, WebUI handlers were being disabled via DisallowJavascript when the user clicked the browser's Back and Forward buttons, even for in-page navigations (fragments or window.history.push). That led to crashes and unexpected behavior. BUG=621885 R=creis@chromium.org, dbeam@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/e525223a1c71de1106cf8e23c54d... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as e525223a1c71de1106cf8e23c54d239d298d9982 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
