Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2615253002:
Resolve ready and finished promises asynchronously (Closed)
|
Created:
3 years, 11 months ago by adithyas Modified:
3 years, 11 months ago CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-bindings_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionResolve ready and finished promises asynchronously
~PlayStateUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayStateUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous.
The DCHECK added crashes the following tests (when the promises are resolved synchronously):
web-animations-api/animation-ready-promise.html
animations/filter-responsive-neutral-keyframe.html
web-animations-api/playState-changes.html
imported/wpt/web-animations/interfaces/Animation/finished.html
imported/wpt/web-animations/animation-model/keyframe-effects/effect-value-context.html
web-animations-api/startTime.html
imported/wpt/web-animations/interfaces/Animation/playState.html
animations/viewport-unit-animation-responsive.html
imported/wpt/web-animations/interfaces/Animation/cancel.html
animations/zoom-responsive-transform-animation.html
imported/wpt/web-animations/interfaces/Animation/play.html
animations/transform-responsive-neutral-keyframe.html
animations/opacity-responsive-neutral-keyframe.html
imported/wpt/web-animations/interfaces/Animation/onfinish.html
imported/wpt/web-animations/timing-model/animations/updating-the-finished-state.html
transitions/opacity-transition-zindex.html
transitions/opacity-transform-transitions-inside-iframe.html
virtual/threaded/animations/transitions-retarget.html
imported/wpt/web-animations/interfaces/Animation/startTime.html
imported/wpt/web-animations/interfaces/Animation/playbackRate.html
imported/wpt/web-animations/interfaces/Animation/pause.html
web-animations-api/animation-set-timeline.html
BUG=678706
Review-Url: https://codereview.chromium.org/2615253002
Cr-Commit-Position: refs/heads/master@{#442272}
Committed: https://chromium.googlesource.com/chromium/src/+/ffb284d85f2e725aefb2314d436addf9431622ad
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix nits #
Messages
Total messages: 29 (17 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.
Description was changed from ========== Resolve ready and finished promises asynchronously BUG=678706 ========== to ========== Resolve ready and finished promises asynchronously ~PlayUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous. BUG=678706 ==========
adithyas@chromium.org changed reviewers: + alancutter@chromium.org, esprehn@chromium.org, jbroman@chromium.org
Can we get a test? :) Also is the promise resolving code hardened against the context being disposed between the resolveAsync call? You might need null checks or isContextDestroyed checks. https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h (right): https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h:108: DCHECK(!ScriptForbiddenScope::isScriptForbidden()); We usually RELEASE_ASSERT/CHECK this instead so it doesn't let security bugs through.
https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h (right): https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h:108: DCHECK(!ScriptForbiddenScope::isScriptForbidden()); On 2017/01/06 at 21:10:25, esprehn wrote: > We usually RELEASE_ASSERT/CHECK this instead so it doesn't let security bugs through. I plan on adding a RELEASE_ASSERT in a different patch. I added the DCHECK as a regression test, a few of the web-animation-api/ and animation/ layout tests fail with this added in (if the promises are resolved synchronously). Also, the execution context checks are right below this line :)
On 2017/01/06 at 21:33:41, adithyas wrote: > https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h (right): > > https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h:108: DCHECK(!ScriptForbiddenScope::isScriptForbidden()); > On 2017/01/06 at 21:10:25, esprehn wrote: > > We usually RELEASE_ASSERT/CHECK this instead so it doesn't let security bugs through. > > I plan on adding a RELEASE_ASSERT in a different patch. I added the DCHECK as a regression test, a few of the web-animation-api/ and animation/ layout tests fail with this added in (if the promises are resolved synchronously). > > Also, the execution context checks are right below this line :) Word. Can you mention which tests apply in the change description too? Lgtm
Description was changed from ========== Resolve ready and finished promises asynchronously ~PlayUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous. BUG=678706 ========== to ========== Resolve ready and finished promises asynchronously ~PlayUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous. The DCHECK added crashes the following tests (when the promises are resolved synchronously): web-animations-api/animation-ready-promise.html animations/filter-responsive-neutral-keyframe.html web-animations-api/playState-changes.html imported/wpt/web-animations/interfaces/Animation/finished.html imported/wpt/web-animations/animation-model/keyframe-effects/effect-value-context.html web-animations-api/startTime.html imported/wpt/web-animations/interfaces/Animation/playState.html animations/viewport-unit-animation-responsive.html imported/wpt/web-animations/interfaces/Animation/cancel.html animations/zoom-responsive-transform-animation.html imported/wpt/web-animations/interfaces/Animation/play.html animations/transform-responsive-neutral-keyframe.html animations/opacity-responsive-neutral-keyframe.html imported/wpt/web-animations/interfaces/Animation/onfinish.html imported/wpt/web-animations/timing-model/animations/updating-the-finished-state.html transitions/opacity-transition-zindex.html transitions/opacity-transform-transitions-inside-iframe.html virtual/threaded/animations/transitions-retarget.html imported/wpt/web-animations/interfaces/Animation/startTime.html imported/wpt/web-animations/interfaces/Animation/playbackRate.html imported/wpt/web-animations/interfaces/Animation/pause.html web-animations-api/animation-set-timeline.html BUG=678706 ==========
What is PlayUpdateScope? I cannot find it in the code base. In general, it's not a good thing to resolve/reject a promise in a destructor since there's no guarantee about the timing when the destructor gets called (i.e., it may expose GC behaviors to the web).
On 2017/01/07 at 01:22:06, haraken wrote: > What is PlayUpdateScope? I cannot find it in the code base. > > In general, it's not a good thing to resolve/reject a promise in a destructor since there's no guarantee about the timing when the destructor gets called (i.e., it may expose GC behaviors to the web). It's a stack allocated scope, it's a pretty common way to do work in the destructor.
lgtm w/ nits https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/Animation.cpp (right): https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/Animation.cpp:1119: WTF::bind(&AnimationPromise::resolve<Member<Animation>>, nit: Similarly, I'd expect this to be AnimationPromise::resolve<Animation*>, so that the raw pointer, rather than Member object, is passed as the argument. https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/Animation.h (right): https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/Animation.h:229: void resolvePromiseAsync(Member<AnimationPromise>); nit: For Oilpan objects, we usually pass T* rather than Member<T> as arguments. I'd make this AnimationPromise*, and move the ".get()" to the call sites.
On 2017/01/07 02:21:21, esprehn wrote: > On 2017/01/07 at 01:22:06, haraken wrote: > > What is PlayUpdateScope? I cannot find it in the code base. Ah, you meant PlayStateUpdateScope. > > In general, it's not a good thing to resolve/reject a promise in a destructor > since there's no guarantee about the timing when the destructor gets called > (i.e., it may expose GC behaviors to the web). > > It's a stack allocated scope, it's a pretty common way to do work in the > destructor. Yes, LGTM.
alancutter@chromium.org changed reviewers: + haraken@chromium.org
lgtm. https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/Animation.h (right): https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/Animation.h:227: AnimationPromise; Nit: "using =" might be easier to read here with the line wrapping.
Description was changed from ========== Resolve ready and finished promises asynchronously ~PlayUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous. The DCHECK added crashes the following tests (when the promises are resolved synchronously): web-animations-api/animation-ready-promise.html animations/filter-responsive-neutral-keyframe.html web-animations-api/playState-changes.html imported/wpt/web-animations/interfaces/Animation/finished.html imported/wpt/web-animations/animation-model/keyframe-effects/effect-value-context.html web-animations-api/startTime.html imported/wpt/web-animations/interfaces/Animation/playState.html animations/viewport-unit-animation-responsive.html imported/wpt/web-animations/interfaces/Animation/cancel.html animations/zoom-responsive-transform-animation.html imported/wpt/web-animations/interfaces/Animation/play.html animations/transform-responsive-neutral-keyframe.html animations/opacity-responsive-neutral-keyframe.html imported/wpt/web-animations/interfaces/Animation/onfinish.html imported/wpt/web-animations/timing-model/animations/updating-the-finished-state.html transitions/opacity-transition-zindex.html transitions/opacity-transform-transitions-inside-iframe.html virtual/threaded/animations/transitions-retarget.html imported/wpt/web-animations/interfaces/Animation/startTime.html imported/wpt/web-animations/interfaces/Animation/playbackRate.html imported/wpt/web-animations/interfaces/Animation/pause.html web-animations-api/animation-set-timeline.html BUG=678706 ========== to ========== Resolve ready and finished promises asynchronously ~PlayStateUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayStateUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous. The DCHECK added crashes the following tests (when the promises are resolved synchronously): web-animations-api/animation-ready-promise.html animations/filter-responsive-neutral-keyframe.html web-animations-api/playState-changes.html imported/wpt/web-animations/interfaces/Animation/finished.html imported/wpt/web-animations/animation-model/keyframe-effects/effect-value-context.html web-animations-api/startTime.html imported/wpt/web-animations/interfaces/Animation/playState.html animations/viewport-unit-animation-responsive.html imported/wpt/web-animations/interfaces/Animation/cancel.html animations/zoom-responsive-transform-animation.html imported/wpt/web-animations/interfaces/Animation/play.html animations/transform-responsive-neutral-keyframe.html animations/opacity-responsive-neutral-keyframe.html imported/wpt/web-animations/interfaces/Animation/onfinish.html imported/wpt/web-animations/timing-model/animations/updating-the-finished-state.html transitions/opacity-transition-zindex.html transitions/opacity-transform-transitions-inside-iframe.html virtual/threaded/animations/transitions-retarget.html imported/wpt/web-animations/interfaces/Animation/startTime.html imported/wpt/web-animations/interfaces/Animation/playbackRate.html imported/wpt/web-animations/interfaces/Animation/pause.html web-animations-api/animation-set-timeline.html BUG=678706 ==========
https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/Animation.h (right): https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/Animation.h:227: AnimationPromise; On 2017/01/09 at 03:25:07, alancutter wrote: > Nit: "using =" might be easier to read here with the line wrapping. Done. https://codereview.chromium.org/2615253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/Animation.h:229: void resolvePromiseAsync(Member<AnimationPromise>); On 2017/01/07 at 04:06:36, jbroman wrote: > nit: For Oilpan objects, we usually pass T* rather than Member<T> as arguments. I'd make this AnimationPromise*, and move the ".get()" to the call sites. Done.
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.
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, esprehn@chromium.org, jbroman@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2615253002/#ps20001 (title: "Fix nits")
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": 1483981071234420,
"parent_rev": "eb2e0d69c4b49d1ac044feff8563cdb794960b8e", "commit_rev":
"ffb284d85f2e725aefb2314d436addf9431622ad"}
Message was sent while issue was closed.
Description was changed from ========== Resolve ready and finished promises asynchronously ~PlayStateUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayStateUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous. The DCHECK added crashes the following tests (when the promises are resolved synchronously): web-animations-api/animation-ready-promise.html animations/filter-responsive-neutral-keyframe.html web-animations-api/playState-changes.html imported/wpt/web-animations/interfaces/Animation/finished.html imported/wpt/web-animations/animation-model/keyframe-effects/effect-value-context.html web-animations-api/startTime.html imported/wpt/web-animations/interfaces/Animation/playState.html animations/viewport-unit-animation-responsive.html imported/wpt/web-animations/interfaces/Animation/cancel.html animations/zoom-responsive-transform-animation.html imported/wpt/web-animations/interfaces/Animation/play.html animations/transform-responsive-neutral-keyframe.html animations/opacity-responsive-neutral-keyframe.html imported/wpt/web-animations/interfaces/Animation/onfinish.html imported/wpt/web-animations/timing-model/animations/updating-the-finished-state.html transitions/opacity-transition-zindex.html transitions/opacity-transform-transitions-inside-iframe.html virtual/threaded/animations/transitions-retarget.html imported/wpt/web-animations/interfaces/Animation/startTime.html imported/wpt/web-animations/interfaces/Animation/playbackRate.html imported/wpt/web-animations/interfaces/Animation/pause.html web-animations-api/animation-set-timeline.html BUG=678706 ========== to ========== Resolve ready and finished promises asynchronously ~PlayStateUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayStateUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous. The DCHECK added crashes the following tests (when the promises are resolved synchronously): web-animations-api/animation-ready-promise.html animations/filter-responsive-neutral-keyframe.html web-animations-api/playState-changes.html imported/wpt/web-animations/interfaces/Animation/finished.html imported/wpt/web-animations/animation-model/keyframe-effects/effect-value-context.html web-animations-api/startTime.html imported/wpt/web-animations/interfaces/Animation/playState.html animations/viewport-unit-animation-responsive.html imported/wpt/web-animations/interfaces/Animation/cancel.html animations/zoom-responsive-transform-animation.html imported/wpt/web-animations/interfaces/Animation/play.html animations/transform-responsive-neutral-keyframe.html animations/opacity-responsive-neutral-keyframe.html imported/wpt/web-animations/interfaces/Animation/onfinish.html imported/wpt/web-animations/timing-model/animations/updating-the-finished-state.html transitions/opacity-transition-zindex.html transitions/opacity-transform-transitions-inside-iframe.html virtual/threaded/animations/transitions-retarget.html imported/wpt/web-animations/interfaces/Animation/startTime.html imported/wpt/web-animations/interfaces/Animation/playbackRate.html imported/wpt/web-animations/interfaces/Animation/pause.html web-animations-api/animation-set-timeline.html BUG=678706 Review-Url: https://codereview.chromium.org/2615253002 Cr-Commit-Position: refs/heads/master@{#442272} Committed: https://chromium.googlesource.com/chromium/src/+/ffb284d85f2e725aefb2314d436a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ffb284d85f2e725aefb2314d436a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
