Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781623006:
MD Settings: Fix Appearance home page URL crash (Closed)
|
Created:
3 years, 8 months ago by tommycli Modified:
3 years, 8 months ago Reviewers:
dpapad CC:
chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, scottchen Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Fix Appearance home page URL crash
The Appearance home page URL validation occurs before the On Startup
section calls 'onStartupPrefsPageLoad', there will be a
IsJavascriptAllowed crash. Having the URL validation function itself
call AllowJavascript within SettingsStartupPagesHandler is problematic
because that handler is so heavy.
This CL moves 'validateStartupPage' to the OnStartupHandler, which is
lightweight and has no hooks to OnJavascriptAllowed, so we can make
'validateStartupPage' call AllowJavascript.
BUG=705366
Review-Url: https://codereview.chromium.org/2781623006
Cr-Commit-Position: refs/heads/master@{#460240}
Committed: https://chromium.googlesource.com/chromium/src/+/9b4945ffcbfc14e7a648e20053ec7909927275c0
Patch Set 1 #
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tommycli@chromium.org changed reviewers: + dpapad@chromium.org
dpapad: PTAL, thanks
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/28 at 18:04:02, tommycli wrote: > dpapad: PTAL, thanks Fix seems reasonable, just a few questions. - Bug number seems wrong, should it be 705366? - Now that 'validateStartupPage' is used by the OnStartupPage as a whole, should we move the JS code to a different proxy from [1] to [2] (I don't think this should be in same CL, just add a TODO if so)? [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... [2] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... - "Having the URL validation function itself call AllowJavascript within SettingsStartupPagesHandler is problematic because that handler is so heavy". Is AllowJavascript() work affected at all by how big the C++ handler is?
On 2017/03/28 at 18:58:00, dpapad wrote: > On 2017/03/28 at 18:04:02, tommycli wrote: > > dpapad: PTAL, thanks > > Fix seems reasonable, just a few questions. > > - Bug number seems wrong, should it be 705366? > - Now that 'validateStartupPage' is used by the OnStartupPage as a whole, should we move the JS code to a different proxy from [1] to [2] (I don't think this should be in same CL, just add a TODO if so)? > > [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > [2] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > > - "Having the URL validation function itself call AllowJavascript within SettingsStartupPagesHandler is problematic because that handler is so heavy". Is AllowJavascript() work affected at all by how big the C++ handler is? Regarding last question, I think I get what you meant now. Calling AllowJavascript() triggers the OnAllowJavascript() method, which does a lot of work for the case of SettingsStartupPagesHandler.
Description was changed from ========== MD Settings: Fix Appearance home page URL crash The Appearance home page URL validation occurs before the On Startup section calls 'onStartupPrefsPageLoad', there will be a IsJavascriptAllowed crash. Having the URL validation function itself call AllowJavascript within SettingsStartupPagesHandler is problematic because that handler is so heavy. This CL moves 'validateStartupPage' to the OnStartupHandler, which is lightweight and has no hooks to OnJavascriptAllowed, so we can make 'validateStartupPage' call AllowJavascript. BUG=704688 ========== to ========== MD Settings: Fix Appearance home page URL crash The Appearance home page URL validation occurs before the On Startup section calls 'onStartupPrefsPageLoad', there will be a IsJavascriptAllowed crash. Having the URL validation function itself call AllowJavascript within SettingsStartupPagesHandler is problematic because that handler is so heavy. This CL moves 'validateStartupPage' to the OnStartupHandler, which is lightweight and has no hooks to OnJavascriptAllowed, so we can make 'validateStartupPage' call AllowJavascript. BUG=705366 ==========
On 2017/03/28 19:08:15, dpapad wrote: > On 2017/03/28 at 18:58:00, dpapad wrote: > > On 2017/03/28 at 18:04:02, tommycli wrote: > > > dpapad: PTAL, thanks > > > > Fix seems reasonable, just a few questions. > > > > - Bug number seems wrong, should it be 705366? > > - Now that 'validateStartupPage' is used by the OnStartupPage as a whole, > should we move the JS code to a different proxy from [1] to [2] (I don't think > this should be in same CL, just add a TODO if so)? > > > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > > [2] > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > > > > - "Having the URL validation function itself call AllowJavascript within > SettingsStartupPagesHandler is problematic because that handler is so heavy". Is > AllowJavascript() work affected at all by how big the C++ handler is? > > Regarding last question, I think I get what you meant now. Calling > AllowJavascript() triggers the OnAllowJavascript() method, which does a lot of > work for the case of SettingsStartupPagesHandler. ^ Exactly. Regarding moving the BrowserProxy call. I think actually it's good where it is, since the JavaScript side is still the same. If anything, the validateStartupPage C++ handler should belong in a separate CommonBrowserProxy C++ class, since it's used by both the On Startup and Appearance sections. -- But it didn't seem worth it to make a whole new handler for just that class.
On 2017/03/28 at 20:10:07, tommycli wrote: > On 2017/03/28 19:08:15, dpapad wrote: > > On 2017/03/28 at 18:58:00, dpapad wrote: > > > On 2017/03/28 at 18:04:02, tommycli wrote: > > > > dpapad: PTAL, thanks > > > > > > Fix seems reasonable, just a few questions. > > > > > > - Bug number seems wrong, should it be 705366? > > > - Now that 'validateStartupPage' is used by the OnStartupPage as a whole, > > should we move the JS code to a different proxy from [1] to [2] (I don't think > > this should be in same CL, just add a TODO if so)? > > > > > > [1] > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > > > [2] > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > > > > > > - "Having the URL validation function itself call AllowJavascript within > > SettingsStartupPagesHandler is problematic because that handler is so heavy". Is > > AllowJavascript() work affected at all by how big the C++ handler is? > > > > Regarding last question, I think I get what you meant now. Calling > > AllowJavascript() triggers the OnAllowJavascript() method, which does a lot of > > work for the case of SettingsStartupPagesHandler. > > ^ Exactly. > > Regarding moving the BrowserProxy call. I think actually it's good where it is, since the JavaScript side is still the same. If anything, the validateStartupPage C++ handler should belong in a separate CommonBrowserProxy C++ class, since it's used by both the On Startup and Appearance sections. > > -- But it didn't seem worth it to make a whole new handler for just that class. LGTM We actually contact the same C++ handler from 2 different proxies, see https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear... which is a bit odd, but not causing any issues otherwise AFAIK.
On 2017/03/28 20:30:34, dpapad wrote: > On 2017/03/28 at 20:10:07, tommycli wrote: > > On 2017/03/28 19:08:15, dpapad wrote: > > > On 2017/03/28 at 18:58:00, dpapad wrote: > > > > On 2017/03/28 at 18:04:02, tommycli wrote: > > > > > dpapad: PTAL, thanks > > > > > > > > Fix seems reasonable, just a few questions. > > > > > > > > - Bug number seems wrong, should it be 705366? > > > > - Now that 'validateStartupPage' is used by the OnStartupPage as a whole, > > > should we move the JS code to a different proxy from [1] to [2] (I don't > think > > > this should be in same CL, just add a TODO if so)? > > > > > > > > [1] > > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > > > > [2] > > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > > > > > > > > - "Having the URL validation function itself call AllowJavascript within > > > SettingsStartupPagesHandler is problematic because that handler is so > heavy". Is > > > AllowJavascript() work affected at all by how big the C++ handler is? > > > > > > Regarding last question, I think I get what you meant now. Calling > > > AllowJavascript() triggers the OnAllowJavascript() method, which does a lot > of > > > work for the case of SettingsStartupPagesHandler. > > > > ^ Exactly. > > > > Regarding moving the BrowserProxy call. I think actually it's good where it > is, since the JavaScript side is still the same. If anything, the > validateStartupPage C++ handler should belong in a separate CommonBrowserProxy > C++ class, since it's used by both the On Startup and Appearance sections. > > > > -- But it didn't seem worth it to make a whole new handler for just that > class. > > LGTM > > We actually contact the same C++ handler from 2 different proxies, see > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/on_sta... > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear... > which is a bit odd, but not causing any issues otherwise AFAIK. Thanks! sending it in...
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
CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by tommycli@chromium.org to run a CQ dry run
Dry run: 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 tommycli@chromium.org
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...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1490743765871900, "parent_rev":
"546b3e91945b120a9540fc1bf9b53719ad9e64b5", "commit_rev":
"9b4945ffcbfc14e7a648e20053ec7909927275c0"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Fix Appearance home page URL crash The Appearance home page URL validation occurs before the On Startup section calls 'onStartupPrefsPageLoad', there will be a IsJavascriptAllowed crash. Having the URL validation function itself call AllowJavascript within SettingsStartupPagesHandler is problematic because that handler is so heavy. This CL moves 'validateStartupPage' to the OnStartupHandler, which is lightweight and has no hooks to OnJavascriptAllowed, so we can make 'validateStartupPage' call AllowJavascript. BUG=705366 ========== to ========== MD Settings: Fix Appearance home page URL crash The Appearance home page URL validation occurs before the On Startup section calls 'onStartupPrefsPageLoad', there will be a IsJavascriptAllowed crash. Having the URL validation function itself call AllowJavascript within SettingsStartupPagesHandler is problematic because that handler is so heavy. This CL moves 'validateStartupPage' to the OnStartupHandler, which is lightweight and has no hooks to OnJavascriptAllowed, so we can make 'validateStartupPage' call AllowJavascript. BUG=705366 Review-Url: https://codereview.chromium.org/2781623006 Cr-Commit-Position: refs/heads/master@{#460240} Committed: https://chromium.googlesource.com/chromium/src/+/9b4945ffcbfc14e7a648e20053ec... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/9b4945ffcbfc14e7a648e20053ec... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
