Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936213002:
[Extensions] Don't require() a module for calling a method (Closed)
Description[Extensions] Don't require() a module for calling a method
The ModuleSystem has an API to call a method of a given module, which
many callers use as a way to call into JS. We've updated the JS call
itself to accept asynchronous JS execution, but if the module isn't
already active, the Require() call can result in JS execution.
Modify CallModuleMethodSafe() so that it won't Require() a module that
doesn't exist. I think this should be okay, since I can't find any
cases of calling into a module where we'd still want to if the module
wasn't required. The main time we would attempt to call into a module
that hasn't been required is to dispatch an event in JS bindings, but
if the Event module has never been instantiated, it's impossible that
any events have been registered.
Gracefully handle the case of the module not being required, and just
don't dispatch the method.
BUG=629431
Review-Url: https://codereview.chromium.org/2936213002
Cr-Commit-Position: refs/heads/master@{#482119}
Committed: https://chromium.googlesource.com/chromium/src/+/fedbe848f3024dd690f93545a337a2a6fb2aa81f
Patch Set 1 : . #
Total comments: 7
Patch Set 2 : lazyboy's #
Messages
Total messages: 39 (33 generated)
The CQ bit was checked by rdevlin.cronin@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 commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by rdevlin.cronin@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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@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 commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by rdevlin.cronin@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 commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by rdevlin.cronin@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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== try BUG= ========== to ========== [Extensions] Don't require() a module for calling a method The ModuleSystem has an API to call a method of a given module, which many callers use as a way to call into JS. We've updated the JS call itself to accept asynchronous JS execution, but if the module isn't already active, the Require() call can result in JS execution. Modify CallModuleMethodSafe() so that it won't Require() a module that doesn't exist. I think this should be okay, since I can't find any cases of calling into a module where we'd still want to if the module wasn't required. The main time we would attempt to call into a module that hasn't been required is to dispatch an event in JS bindings, but if the Event module has never been instantiated, it's impossible that any events have been registered. Gracefully handle the case of the module not being required, and just don't dispatch the method. BUG=629431 ==========
rdevlin.cronin@chromium.org changed reviewers: + jbroman@chromium.org, lazyboy@chromium.org
And another fun one for you... I *think* this is okay, and at least no tests fail, but if you can think of exceptions to this, lemme know! https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... extensions/renderer/module_system.cc:332: return; Note: I'll add a comment explaining when this can legitimately happen.
This seems reasonable to me, but I don't know enough about how ModuleSystem works to know. Deferring to lazyboy@.
lgtm https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... extensions/renderer/module_system.cc:332: return; On 2017/06/16 14:53:16, Devlin wrote: > Note: I'll add a comment explaining when this can legitimately happen. OK, I'd be interested to know. https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... extensions/renderer/module_system.cc:877: if (!module.IsEmpty() && module->IsUndefined()) Add a note what this (!IsEmpty but IsUndefined = true) signifies. https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... extensions/renderer/module_system.cc:878: return function; nit: do you think it makes it more readable to "return v8::Local<v8::Function>()" for empty cases here and in lines 866, 883 etc. Otherwise, reader has to scan lines above to check what |function| contains... |function| is only set in line 894 and its declaration can be moved there.
The CQ bit was checked by rdevlin.cronin@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...
https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... extensions/renderer/module_system.cc:332: return; On 2017/06/23 21:28:13, lazyboy wrote: > On 2017/06/16 14:53:16, Devlin wrote: > > Note: I'll add a comment explaining when this can legitimately happen. > > OK, I'd be interested to know. Done. https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... extensions/renderer/module_system.cc:877: if (!module.IsEmpty() && module->IsUndefined()) On 2017/06/23 21:28:13, lazyboy wrote: > Add a note what this (!IsEmpty but IsUndefined = true) signifies. Done. https://codereview.chromium.org/2936213002/diff/80001/extensions/renderer/mod... extensions/renderer/module_system.cc:878: return function; On 2017/06/23 21:28:13, lazyboy wrote: > nit: do you think it makes it more readable to "return > v8::Local<v8::Function>()" for empty cases here and in lines 866, 883 etc. > Otherwise, reader has to scan lines above to check what |function| contains... > |function| is only set in line 894 and its declaration can be moved there. Sure, that's reasonable. It means we lose RVO, but that shouldn't matter with v8::Locals.
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2936213002/#ps100001 (title: "lazyboy's")
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": 100001, "attempt_start_ts": 1498269901108740,
"parent_rev": "d701d8e63d28192afbdbe9d0d65569a4b994fd51", "commit_rev":
"fedbe848f3024dd690f93545a337a2a6fb2aa81f"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Don't require() a module for calling a method The ModuleSystem has an API to call a method of a given module, which many callers use as a way to call into JS. We've updated the JS call itself to accept asynchronous JS execution, but if the module isn't already active, the Require() call can result in JS execution. Modify CallModuleMethodSafe() so that it won't Require() a module that doesn't exist. I think this should be okay, since I can't find any cases of calling into a module where we'd still want to if the module wasn't required. The main time we would attempt to call into a module that hasn't been required is to dispatch an event in JS bindings, but if the Event module has never been instantiated, it's impossible that any events have been registered. Gracefully handle the case of the module not being required, and just don't dispatch the method. BUG=629431 ========== to ========== [Extensions] Don't require() a module for calling a method The ModuleSystem has an API to call a method of a given module, which many callers use as a way to call into JS. We've updated the JS call itself to accept asynchronous JS execution, but if the module isn't already active, the Require() call can result in JS execution. Modify CallModuleMethodSafe() so that it won't Require() a module that doesn't exist. I think this should be okay, since I can't find any cases of calling into a module where we'd still want to if the module wasn't required. The main time we would attempt to call into a module that hasn't been required is to dispatch an event in JS bindings, but if the Event module has never been instantiated, it's impossible that any events have been registered. Gracefully handle the case of the module not being required, and just don't dispatch the method. BUG=629431 Review-Url: https://codereview.chromium.org/2936213002 Cr-Commit-Position: refs/heads/master@{#482119} Committed: https://chromium.googlesource.com/chromium/src/+/fedbe848f3024dd690f93545a337... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:100001) as https://chromium.googlesource.com/chromium/src/+/fedbe848f3024dd690f93545a337... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
