Post task when rejecting Animation promises inside ScriptForbiddenScope

Post task when rejecting Animation promises inside ScriptForbiddenScope

It is possible for the ready promise to be rejected in a ScriptForbiddenScope. Rejecting a promise with a DOMException results in the constructor for DOMException being executed (due to a ToV8 call), which triggers a RELEASE_ASSERT that detects if script is being executed in a forbidden scope. This patch posts a task to reject promises when inside a forbidden scope to prevent the crash.

BUG=701631
Review-Url: https://codereview.chromium.org/2785303002
Cr-Commit-Position: refs/heads/master@{#470391}
Committed: https://chromium.googlesource.com/chromium/src/+/13c178302b8a4ce77dc493127b1a9eff97c4e7c2

[adithyas, 2017-03-30 21:25:26]

The CQ bit was checked by adithyas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 21:26:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 23:17:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 23:17:20]

Dry run: This issue passed the CQ dry run.

[adithyas, 2017-04-26 18:18:59]

adithyas@chromium.org changed reviewers:
+ alancutter@chromium.org, haraken@chromium.org

[haraken, 2017-04-27 00:17:14]

https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:1153: // Animation
promises can be rejected inside a ScriptForbiddenScope. When

Would you help me understand where the ScriptForbiddenScope is set?

https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:1157: // (and cannot be
overwritten), we can allow it to run in a forbidden scope.

Your analysis looks correct but for safety I'd prefer not allowing the script
execution while you're in ScriptForbiddenScope.

If you really need to reject the promise here, would it be an option to post a
task to reject the promise (which will run after the ScriptForbiddenScope
exits)?

[adithyas, 2017-04-27 19:09:52]

https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:1153: // Animation
promises can be rejected inside a ScriptForbiddenScope. When
On 2017/04/27 at 00:17:13, haraken wrote:
> Would you help me understand where the ScriptForbiddenScope is set?

It's set in Document::updateStyleAndLayout:
https://chromium.googlesource.com/chromium/src/+/9afc2bc78b53f5439bce60e2de93...

https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:1157: // (and cannot be
overwritten), we can allow it to run in a forbidden scope.
On 2017/04/27 at 00:17:13, haraken wrote:
> Your analysis looks correct but for safety I'd prefer not allowing the script
execution while you're in ScriptForbiddenScope.
> 
> If you really need to reject the promise here, would it be an option to post a
task to reject the promise (which will run after the ScriptForbiddenScope
exits)?

I've updated the CL to do this (i.e. post a task if inside a forbidden scope),
but I think it's simpler to reject the promise synchronously (considering we're
sure that no script is actually being executed).

[haraken, 2017-04-28 01:57:14]

On 2017/04/27 19:09:52, adithyas wrote:
>
https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/animation/Animation.cpp (right):
> 
>
https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/animation/Animation.cpp:1153: // Animation
> promises can be rejected inside a ScriptForbiddenScope. When
> On 2017/04/27 at 00:17:13, haraken wrote:
> > Would you help me understand where the ScriptForbiddenScope is set?
> 
> It's set in Document::updateStyleAndLayout:
>
https://chromium.googlesource.com/chromium/src/+/9afc2bc78b53f5439bce60e2de93...
> 
>
https://codereview.chromium.org/2785303002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/animation/Animation.cpp:1157: // (and cannot be
> overwritten), we can allow it to run in a forbidden scope.
> On 2017/04/27 at 00:17:13, haraken wrote:
> > Your analysis looks correct but for safety I'd prefer not allowing the
script
> execution while you're in ScriptForbiddenScope.
> > 
> > If you really need to reject the promise here, would it be an option to post
a
> task to reject the promise (which will run after the ScriptForbiddenScope
> exits)?
> 
> I've updated the CL to do this (i.e. post a task if inside a forbidden scope),
> but I think it's simpler to reject the promise synchronously (considering
we're
> sure that no script is actually being executed).

For safety, I'd prefer avoiding using AllowUserAgentScript even if you're sure
that it's safe for some complex reason.

Also regarding this Animation case, the asynchronous promise rejection is
consistent with the asynchronous promise resolution (which we're already doing).

LGTM.

[alancutter (OOO until 2018), 2017-05-08 07:26:13]

The description says this fixes a crash, can we add a test case?

[adithyas, 2017-05-08 15:03:31]

Description was changed from

==========
Add AllowUserAgentScript before rejecting Animation promises

It is possible for the ready promise to be rejected in a ScriptForbiddenScope.
Rejecting a promise with a DOMException results in the constructor for
DOMException being executed (due to a ToV8 call), which triggers a
RELEASE_ASSERT that detects if script is being executed in a forbidden scope.
Executing the constructor in a forbidden scope should be safe (the constructor
is not user defined), so this patch adds an AllowUserAgentScript to prevent the
crash.

BUG=701631
==========

to

==========
Post task when rejecting Animation promises inside ScriptForbiddenScope

It is possible for the ready promise to be rejected in a ScriptForbiddenScope.
Rejecting a promise with a DOMException results in the constructor for
DOMException being executed (due to a ToV8 call), which triggers a
RELEASE_ASSERT that detects if script is being executed in a forbidden scope.
This patch posts a task to reject promises when inside a forbidden scope to
prevent the crash.

BUG=701631
==========

[adithyas, 2017-05-08 15:04:31]

On 2017/05/08 at 07:26:13, alancutter wrote:
> The description says this fixes a crash, can we add a test case?

Yup, added a test case. Also updated the title and description of the CL.

[alancutter (OOO until 2018), 2017-05-08 23:49:14]

lgtm

[adithyas, 2017-05-09 14:25:09]

The CQ bit was checked by adithyas@chromium.org

[adithyas, 2017-05-09 14:25:10]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2785303002/#ps60001
(title: "Add layout test")

[commit-bot: I haz the power, 2017-05-09 14:25:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 18:48:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494339909816050,
"parent_rev": "dc2e682ca5558e90332f8443e23539a04f436273", "commit_rev":
"13c178302b8a4ce77dc493127b1a9eff97c4e7c2"}

[commit-bot: I haz the power, 2017-05-09 18:48:18]

Description was changed from

==========
Post task when rejecting Animation promises inside ScriptForbiddenScope

It is possible for the ready promise to be rejected in a ScriptForbiddenScope.
Rejecting a promise with a DOMException results in the constructor for
DOMException being executed (due to a ToV8 call), which triggers a
RELEASE_ASSERT that detects if script is being executed in a forbidden scope.
This patch posts a task to reject promises when inside a forbidden scope to
prevent the crash.

BUG=701631
==========

to

==========
Post task when rejecting Animation promises inside ScriptForbiddenScope

It is possible for the ready promise to be rejected in a ScriptForbiddenScope.
Rejecting a promise with a DOMException results in the constructor for
DOMException being executed (due to a ToV8 call), which triggers a
RELEASE_ASSERT that detects if script is being executed in a forbidden scope.
This patch posts a task to reject promises when inside a forbidden scope to
prevent the crash.

BUG=701631

Review-Url: https://codereview.chromium.org/2785303002
Cr-Commit-Position: refs/heads/master@{#470391}
Committed:
https://chromium.googlesource.com/chromium/src/+/13c178302b8a4ce77dc493127b1a...
==========

[commit-bot: I haz the power, 2017-05-09 18:48:19]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/13c178302b8a4ce77dc493127b1a...