Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644343002:
Add AllowUserAgentScript in ScriptPromiseResolver::resolveOrReject (Closed)
DescriptionAdd AllowUserAgentScript in ScriptPromiseResolver::resolveOrReject
BUG=679648, 676004
Review-Url: https://codereview.chromium.org/2644343002
Cr-Commit-Position: refs/heads/master@{#448711}
Committed: https://chromium.googlesource.com/chromium/src/+/b81015e872d51319648e448129becd5a19799848
Patch Set 1 #Patch Set 2 : Remove isScriptForbidden check in HTMLMediaElement #
Total comments: 3
Messages
Total messages: 24 (11 generated)
The CQ bit was checked by adithyas@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.
adithyas@chromium.org changed reviewers: + esprehn@chromium.org, haraken@chromium.org, jbroman@chromium.org
lgtm
adithyas@chromium.org changed reviewers: + a.obzhirov@samsung.com
+a.obzhirov: Looks like a duplicate issue was filed. I removed the check you added in HTMLMediaElement (http://crrev.com/2594353003), PTAL.
Description was changed from ========== Add AllowUserAgentScript in ScriptPromiseResolver::resolveOrReject BUG=679648 ========== to ========== Add AllowUserAgentScript in ScriptPromiseResolver::resolveOrReject BUG=679648,676004 ==========
https://codereview.chromium.org/2644343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptPromiseResolver.h (right): https://codereview.chromium.org/2644343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptPromiseResolver.h:139: ToV8(value, m_scriptState->context()->Global(), Are we pretty sure that we have no way to invoke a user script in ToV8? (I just don't know. JS is a very flexible language :-) https://codereview.chromium.org/2644343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (left): https://codereview.chromium.org/2644343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:833: rejectPlayPromises( I'm asking the question in https://codereview.chromium.org/2594353003/, but I'm wondering why we cannot schedule an async task for rejectPlayPromises in this case.
On 2017/01/20 19:34:56, adithyas wrote: > +a.obzhirov: Looks like a duplicate issue was filed. I removed the check you > added in HTMLMediaElement (http://crrev.com/2594353003), PTAL. Thanks, there was also another issue about audio element crash with similar call stack, cannot find any more.
https://codereview.chromium.org/2644343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptPromiseResolver.h (right): https://codereview.chromium.org/2644343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptPromiseResolver.h:139: ToV8(value, m_scriptState->context()->Global(), On 2017/01/21 at 01:51:09, haraken wrote: > Are we pretty sure that we have no way to invoke a user script in ToV8? (I just don't know. JS is a very flexible language :-) If I understand correctly, the constructors called by this function are native constructors, which cannot be replaced by user script (or rather, I don't know how to replace these constructors with script). So I'm not sure how you could invoke user script here.
haraken@: Can I commit this patch?
On 2017/02/02 17:48:42, adithyas wrote: > haraken@: Can I commit this patch? Sorry for the review delay. The crash has been already fixed by https://codereview.chromium.org/2594353003/, hasn't it? (Just to clarify, I don't think https://codereview.chromium.org/2594353003/ is a right fix -- I think we should post an async task and run rejectPlayPromises().)
On 2017/02/03 at 04:25:38, haraken wrote: > On 2017/02/02 17:48:42, adithyas wrote: > > haraken@: Can I commit this patch? > > Sorry for the review delay. The crash has been already fixed by https://codereview.chromium.org/2594353003/, hasn't it? > > (Just to clarify, I don't think https://codereview.chromium.org/2594353003/ is a right fix -- I think we should post an async task and run rejectPlayPromises().) Yes, I think the crash is fixed by that patch. However, the ScriptForbiddenScope::IsScriptForbidden() check inside resolveOrReject is redundant if calling ToV8 in a forbidden scope triggers an assert and crashes. So I think it would make sense to either assert that script is not forbidden in resolveOrReject, or to do what this patch does (allow user agent script for the ToV8 call which I think is safe, and then post a task if script is forbidden).
On 2017/02/06 15:09:47, adithyas wrote: > On 2017/02/03 at 04:25:38, haraken wrote: > > On 2017/02/02 17:48:42, adithyas wrote: > > > haraken@: Can I commit this patch? > > > > Sorry for the review delay. The crash has been already fixed by > https://codereview.chromium.org/2594353003/, hasn't it? > > > > (Just to clarify, I don't think https://codereview.chromium.org/2594353003/ is > a right fix -- I think we should post an async task and run > rejectPlayPromises().) > > Yes, I think the crash is fixed by that patch. However, the > ScriptForbiddenScope::IsScriptForbidden() check inside resolveOrReject is > redundant if calling ToV8 in a forbidden scope triggers an assert and crashes. > So I think it would make sense to either assert that script is not forbidden in > resolveOrReject, or to do what this patch does (allow user agent script for the > ToV8 call which I think is safe, and then post a task if script is forbidden). LGTM as a short-term crash fix, but I really think that a right fix would be to post a task for rejectPlayPromises if script execution is forbidden. We're already doing that in a number of places in the code base.
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2644343002/#ps20001 (title: "Remove isScriptForbidden check in HTMLMediaElement")
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": 20001, "attempt_start_ts": 1486491282471630,
"parent_rev": "4196987e022db4f94e9c2fb566a1c8ac585420ab", "commit_rev":
"b81015e872d51319648e448129becd5a19799848"}
Message was sent while issue was closed.
Description was changed from ========== Add AllowUserAgentScript in ScriptPromiseResolver::resolveOrReject BUG=679648,676004 ========== to ========== Add AllowUserAgentScript in ScriptPromiseResolver::resolveOrReject BUG=679648,676004 Review-Url: https://codereview.chromium.org/2644343002 Cr-Commit-Position: refs/heads/master@{#448711} Committed: https://chromium.googlesource.com/chromium/src/+/b81015e872d51319648e448129be... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b81015e872d51319648e448129be... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
