Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250433005:
Simplify security token setting logic in WindowProxy (Closed)
DescriptionSimplify security token setting logic in WindowProxy
Since r412195, Blink uses a different path to notify that the
initial document was accessed: it no longer relies on the fact
that WindowProxy forced v8 to fallback to canAccess().
BUG=none
Committed: https://crrev.com/fbd849942b44c5923cf15f29cab1c4771d121dcb
Cr-Commit-Position: refs/heads/master@{#412736}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 1
Messages
Total messages: 25 (16 generated)
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 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...
Description was changed from ========== Check if we still need to fall back to canAccess() checks (probably) BUG= ========== to ========== Simplify the "delay set" security token logic in WindowProxy Since r412195, Blink uses a different path to notify that the initial document was accessed: it no longer relies on the fact that WindowProxy forced v8 to fallback to canAccess(). BUG=none ==========
dcheng@chromium.org changed reviewers: + yukishiino@chromium.org
Description was changed from ========== Simplify the "delay set" security token logic in WindowProxy Since r412195, Blink uses a different path to notify that the initial document was accessed: it no longer relies on the fact that WindowProxy forced v8 to fallback to canAccess(). BUG=none ========== to ========== Simplify security token setting logic in WindowProxy Since r412195, Blink uses a different path to notify that the initial document was accessed: it no longer relies on the fact that WindowProxy forced v8 to fallback to canAccess(). BUG=none ==========
According to PS2, it's passing all the tests. https://codereview.chromium.org/2250433005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): https://codereview.chromium.org/2250433005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:434: bool delaySet = m_frame->isRemoteFrame() || (m_world->isMainWorld() && origin->domainWasSetInDOM()); Note: I'm not actually sure why the second part of this condition requires that we only delay it for the main world. Does document.domain not affect other worlds?
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...
On 2016/08/18 00:45:12, dcheng wrote: > According to PS2, it's passing all the tests. > > https://codereview.chromium.org/2250433005/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right): > > https://codereview.chromium.org/2250433005/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:434: bool delaySet = > m_frame->isRemoteFrame() || (m_world->isMainWorld() && > origin->domainWasSetInDOM()); > Note: I'm not actually sure why the second part of this condition requires that > we only delay it for the main world. Does document.domain not affect other > worlds? Hmm. The isMainWorld branch was added in https://codereview.chromium.org/261883004/diff/70001/Source/bindings/v8/V8Win.... It looks like that an isolated world ignores document.domain. I'm not sure if it's a bug or an intentional behavior.
This CL LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
Message was sent while issue was closed.
Description was changed from ========== Simplify security token setting logic in WindowProxy Since r412195, Blink uses a different path to notify that the initial document was accessed: it no longer relies on the fact that WindowProxy forced v8 to fallback to canAccess(). BUG=none ========== to ========== Simplify security token setting logic in WindowProxy Since r412195, Blink uses a different path to notify that the initial document was accessed: it no longer relies on the fact that WindowProxy forced v8 to fallback to canAccess(). BUG=none ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Simplify security token setting logic in WindowProxy Since r412195, Blink uses a different path to notify that the initial document was accessed: it no longer relies on the fact that WindowProxy forced v8 to fallback to canAccess(). BUG=none ========== to ========== Simplify security token setting logic in WindowProxy Since r412195, Blink uses a different path to notify that the initial document was accessed: it no longer relies on the fact that WindowProxy forced v8 to fallback to canAccess(). BUG=none Committed: https://crrev.com/fbd849942b44c5923cf15f29cab1c4771d121dcb Cr-Commit-Position: refs/heads/master@{#412736} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fbd849942b44c5923cf15f29cab1c4771d121dcb Cr-Commit-Position: refs/heads/master@{#412736}
Message was sent while issue was closed.
dcheng@, window.document is currently implemented as a simple data property and has no C++ callback. Is securityCheck() in V8Window.cpp still called for window.document with this change?
Message was sent while issue was closed.
On 2016/08/18 12:44:19, Yuki wrote: > dcheng@, window.document is currently implemented as a simple data property and > has no C++ callback. Is securityCheck() in V8Window.cpp still called for > window.document with this change? We do have some tests (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF...) and they still seem to pass... so I guess?
Message was sent while issue was closed.
On 2016/08/18 19:55:59, dcheng wrote: > On 2016/08/18 12:44:19, Yuki wrote: > > dcheng@, window.document is currently implemented as a simple data property > and > > has no C++ callback. Is securityCheck() in V8Window.cpp still called for > > window.document with this change? > > We do have some tests > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF...) > and they still seem to pass... so I guess? Thanks. Then, this looks good to me, too. :) |
