Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740873005:
ExtensionFrameHelper shouldn't trigger initialization of the main world (Closed)
DescriptionExtensionFrameHelper shouldn't trigger initialization of the main world
DidCreateScriptContext() compares the context argument to the main world
script context. However, getting the main world script context will
initialize its WindowProxy, which can corrupt internal state when
swapping frames.
BUG=700077
Review-Url: https://codereview.chromium.org/2740873005
Cr-Commit-Position: refs/heads/master@{#455975}
Committed: https://chromium.googlesource.com/chromium/src/+/128ef2d7248bd291e08ddfa30c8444dc26abc9b8
Patch Set 1 #
Total comments: 2
Messages
Total messages: 23 (11 generated)
dcheng@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
The CQ bit was checked by dcheng@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.
this patch lgtm, but I'm worried this might not be a complete fix. It does seem a bit odd that accessing the main world script context after a context is created results in a crash. Are we worried about the general case when this can happen?
Description was changed from ========== ExtensionFrameHelper shouldn't trigger initialization of the main world DidCreateScriptContext() compares the context argument to the main world script context. However, getting the main world script context will initialize its WindowProxy, which can corrupt internal state when swapping frames. BUG=700077 ========== to ========== ExtensionFrameHelper shouldn't trigger initialization of the main world DidCreateScriptContext() compares the context argument to the main world script context. However, getting the main world script context will initialize its WindowProxy, which can corrupt internal state when swapping frames. BUG=700077 ==========
On 2017/03/10 02:59:39, Devlin wrote: > this patch lgtm, but I'm worried this might not be a complete fix. It does seem > a bit odd that accessing the main world script context after a context is > created results in a crash. Are we worried about the general case when this can > happen? The problem is during frame swap. If the embedder touches other contexts, it causes them to be initialized. However, the logic to swap globals (see WindowProxyManagerBase::setGlobals()) between frames assumes that the contexts won't be initialized before it swaps the state over. If it does, this confuses things and breaks horribly. I plan on adding some Blink CHECKs so any future misuse turns into a crash rather than (potentially) silently failing.
On 2017/03/10 03:02:52, dcheng wrote: > On 2017/03/10 02:59:39, Devlin wrote: > > this patch lgtm, but I'm worried this might not be a complete fix. It does > seem > > a bit odd that accessing the main world script context after a context is > > created results in a crash. Are we worried about the general case when this > can > > happen? > > The problem is during frame swap. If the embedder touches other contexts, it > causes them to be initialized. However, the logic to swap globals (see > WindowProxyManagerBase::setGlobals()) between frames assumes that the contexts > won't be initialized before it swaps the state over. If it does, this confuses > things and breaks horribly. > > I plan on adding some Blink CHECKs so any future misuse turns into a crash > rather than (potentially) silently failing. (And of course, if it reveals crashes, we'll have to fix those as well =)
The CQ bit was checked by dcheng@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": 1489114996722960, "parent_rev":
"10184858622f92e96aa3aa550b72dab2dc2d2eef", "commit_rev":
"128ef2d7248bd291e08ddfa30c8444dc26abc9b8"}
Message was sent while issue was closed.
Description was changed from ========== ExtensionFrameHelper shouldn't trigger initialization of the main world DidCreateScriptContext() compares the context argument to the main world script context. However, getting the main world script context will initialize its WindowProxy, which can corrupt internal state when swapping frames. BUG=700077 ========== to ========== ExtensionFrameHelper shouldn't trigger initialization of the main world DidCreateScriptContext() compares the context argument to the main world script context. However, getting the main world script context will initialize its WindowProxy, which can corrupt internal state when swapping frames. BUG=700077 Review-Url: https://codereview.chromium.org/2740873005 Cr-Commit-Position: refs/heads/master@{#455975} Committed: https://chromium.googlesource.com/chromium/src/+/128ef2d7248bd291e08ddfa30c84... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/128ef2d7248bd291e08ddfa30c84...
Message was sent while issue was closed.
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
Message was sent while issue was closed.
+cc: nhiroki@ FYI. This CL itself LGTM. https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... File extensions/renderer/extension_frame_helper.cc (right): https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... extensions/renderer/extension_frame_helper.cc:30: constexpr int kMainWorldId = 0; side note: Probably, we should get rid of exposing world IDs. +nhiroki@ FYI.
Message was sent while issue was closed.
haraken@chromium.org changed reviewers: + haraken@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... File extensions/renderer/extension_frame_helper.cc (right): https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... extensions/renderer/extension_frame_helper.cc:30: constexpr int kMainWorldId = 0; On 2017/03/10 07:34:46, Yuki wrote: > side note: Probably, we should get rid of exposing world IDs. > +nhiroki@ FYI. Yeah, agreed. Ideally I want to entirely remove world IDs from the embedder side. Alternately can we expose some public API like isMainWorld()?
Message was sent while issue was closed.
On 2017/03/10 09:17:26, haraken wrote: > https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... > File extensions/renderer/extension_frame_helper.cc (right): > > https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... > extensions/renderer/extension_frame_helper.cc:30: constexpr int kMainWorldId = > 0; > On 2017/03/10 07:34:46, Yuki wrote: > > side note: Probably, we should get rid of exposing world IDs. > > +nhiroki@ FYI. > > Yeah, agreed. Ideally I want to entirely remove world IDs from the embedder > side. > > Alternately can we expose some public API like isMainWorld()? Unless we want to wrap v8::Context in a Blink wrapper object, I think it'd be easiest to pass down whether or not it's the main world as an argument to the WebFrameClient callbacks.
Message was sent while issue was closed.
On 2017/03/10 09:23:33, dcheng wrote: > On 2017/03/10 09:17:26, haraken wrote: > > > https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... > > File extensions/renderer/extension_frame_helper.cc (right): > > > > > https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... > > extensions/renderer/extension_frame_helper.cc:30: constexpr int kMainWorldId = > > 0; > > On 2017/03/10 07:34:46, Yuki wrote: > > > side note: Probably, we should get rid of exposing world IDs. > > > +nhiroki@ FYI. > > > > Yeah, agreed. Ideally I want to entirely remove world IDs from the embedder > > side. > > > > Alternately can we expose some public API like isMainWorld()? > > Unless we want to wrap v8::Context in a Blink wrapper object, I think it'd be > easiest to pass down whether or not it's the main world as an argument to the > WebFrameClient callbacks. Agreed.
Message was sent while issue was closed.
On 2017/03/10 09:26:30, haraken wrote: > On 2017/03/10 09:23:33, dcheng wrote: > > On 2017/03/10 09:17:26, haraken wrote: > > > > > > https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... > > > File extensions/renderer/extension_frame_helper.cc (right): > > > > > > > > > https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... > > > extensions/renderer/extension_frame_helper.cc:30: constexpr int kMainWorldId > = > > > 0; > > > On 2017/03/10 07:34:46, Yuki wrote: > > > > side note: Probably, we should get rid of exposing world IDs. > > > > +nhiroki@ FYI. > > > > > > Yeah, agreed. Ideally I want to entirely remove world IDs from the embedder > > > side. > > > > > > Alternately can we expose some public API like isMainWorld()? > > > > Unless we want to wrap v8::Context in a Blink wrapper object, I think it'd be > > easiest to pass down whether or not it's the main world as an argument to the > > WebFrameClient callbacks. > > Agreed. Note that extensions use world ids for more than just the main world; they also rely on them to be able to inject scripts into a consistent (per-extension) isolated world (using requestExecuteScriptInIsolatedWorld).
Message was sent while issue was closed.
On 2017/03/10 15:54:26, Devlin wrote: > On 2017/03/10 09:26:30, haraken wrote: > > On 2017/03/10 09:23:33, dcheng wrote: > > > On 2017/03/10 09:17:26, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... > > > > File extensions/renderer/extension_frame_helper.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2740873005/diff/1/extensions/renderer/extensi... > > > > extensions/renderer/extension_frame_helper.cc:30: constexpr int > kMainWorldId > > = > > > > 0; > > > > On 2017/03/10 07:34:46, Yuki wrote: > > > > > side note: Probably, we should get rid of exposing world IDs. > > > > > +nhiroki@ FYI. > > > > > > > > Yeah, agreed. Ideally I want to entirely remove world IDs from the > embedder > > > > side. > > > > > > > > Alternately can we expose some public API like isMainWorld()? > > > > > > Unless we want to wrap v8::Context in a Blink wrapper object, I think it'd > be > > > easiest to pass down whether or not it's the main world as an argument to > the > > > WebFrameClient callbacks. > > > > Agreed. > > Note that extensions use world ids for more than just the main world; they also > rely on them to be able to inject scripts into a consistent (per-extension) > isolated world (using requestExecuteScriptInIsolatedWorld). Yeah, so this CL is just making the situation a bit worse. So LGTM. Ideally I'd like to encapsulate the ID concept inside Blink. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
