Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2594353003:
Crash in blink::beforeCallEnteredCallback() (Closed)
|
Created:
4 years ago by Anton Obzhirov Modified:
3 years, 11 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCrash in blink::beforeCallEnteredCallback()
Crash occurs because the script is forbidden due to the insertion
of the media element node when the media element is moved to a new document
and the existing play promise gets rejected because of the invoked load.
Check if the script is forbidden to skip reject play promises.
BUG=676004
Review-Url: https://codereview.chromium.org/2594353003
Cr-Commit-Position: refs/heads/master@{#445107}
Committed: https://chromium.googlesource.com/chromium/src/+/87800a0c858e3ee1e2a406b3aae00e89ae7d077f
Patch Set 1 #
Total comments: 10
Patch Set 2 : Updated after review #
Total comments: 1
Patch Set 3 : Crash in blink::beforeCallEnteredCallback() #
Messages
Total messages: 32 (10 generated)
Description was changed from ========== Crash in blink::beforeCallEnteredCallback() Crash occurs when HTMLMediaElement is moved to a new document and play promise gets rejected because of the invoked load. Check if the script is forbidden to schedule reject play promises. BUG=676004 ========== to ========== Crash in blink::beforeCallEnteredCallback() Crash occurs when HTMLMediaElement is moved to a new document and play promise gets rejected because of the invoked load. Check if the script is forbidden to schedule reject play promises. BUG=676004 ==========
a.obzhirov@samsung.com changed reviewers: + jl@opera.com, mlamouri@chromium.org
Tentative patch. As I understand there were already known issues with promises and media element judging by the code comments, this one tries to fix yet another edge case crash.
On 2016/12/22 16:37:33, Anton Obzhirov wrote: > Tentative patch. As I understand there were already known issues with promises > and media element judging by the code comments, this one tries to fix yet > another edge case crash. Oh, I just actually checked https://bugs.chromium.org/p/chromium/issues/detail?id=660382 and there seems to be already workaround in https://chromium.googlesource.com/chromium/src/+/f4d0ee00fa66b781d5b8c5dad488.... However the extra check this patch adds still might be useful.
On 2016/12/22 16:56:02, Anton Obzhirov wrote: > On 2016/12/22 16:37:33, Anton Obzhirov wrote: > > Tentative patch. As I understand there were already known issues with promises > > and media element judging by the code comments, this one tries to fix yet > > another edge case crash. > > Oh, I just actually checked > https://bugs.chromium.org/p/chromium/issues/detail?id=660382 > and there seems to be already workaround > in > https://chromium.googlesource.com/chromium/src/+/f4d0ee00fa66b781d5b8c5dad488.... > > However the extra check this patch adds still might be useful. Actually the workaround which is still not landed is here https://codereview.chromium.org/2450853005. The change I referred before (https://chromiumcodereview.appspot.com/19693009) is basically the one which introduced the root cause of the crash in the first place. So this patch tries to fix it in HTMLMediaElement::invokeLoadAlgorithm(), plz have a look.
https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/crash-in-media-moved-to-newdocument.html (right): https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/crash-in-media-moved-to-newdocument.html:11: if (promise != undefined) { I don't think you need this, it will return a promise. Just do: `media.play().then()` https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/crash-in-media-moved-to-newdocument.html:14: }); Can you make this an async_test and add unreach on the non-expected path and t.done() on the other. https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:806: if (!ScriptForbiddenScope::isScriptForbidden()) { Why is the script forbidden here?
foolip@chromium.org changed reviewers: + foolip@chromium.org
https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:811: scheduleRejectPlayPromises(AbortError); It's fairly likely that it's still forbidden in a moments time, I'd say forget the promises and leave the promises unresolved instead.
Hi guys, thanks for the review, sorry for delay - was on Christmas holidays. https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/crash-in-media-moved-to-newdocument.html (right): https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/crash-in-media-moved-to-newdocument.html:11: if (promise != undefined) { On 2016/12/23 12:00:26, mlamouri wrote: > I don't think you need this, it will return a promise. Just do: > `media.play().then()` ok https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/crash-in-media-moved-to-newdocument.html:14: }); On 2016/12/23 12:00:26, mlamouri wrote: > Can you make this an async_test and add unreach on the non-expected path and > t.done() on the other. Will do. https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:806: if (!ScriptForbiddenScope::isScriptForbidden()) { On 2016/12/23 12:00:26, mlamouri wrote: > Why is the script forbidden here? Plz check https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TreeS... - Script is forbidden to protect against event handlers firing in the middle of rescoping in |didMoveToNewDocument| callbacks. https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:811: scheduleRejectPlayPromises(AbortError); On 2017/01/04 15:49:00, foolip wrote: > It's fairly likely that it's still forbidden in a moments time, I'd say forget > the promises and leave the promises unresolved instead. I see, will remove it.
Updated after review, plz have a look.
lgtm
Do you know where the ScriptForbiddenScope is set? Another option would be to post an async task to resolve/reject the promises.
lgtm % test issue https://codereview.chromium.org/2594353003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/crash-in-media-moved-to-newdocument.html (right): https://codereview.chromium.org/2594353003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/crash-in-media-moved-to-newdocument.html:10: ).catch(t.done(function() { This should be t.step_func_done, t.done() will end the test and at this point is before anything interesting has happened.
On 2017/01/18 10:38:12, haraken wrote: > Do you know where the ScriptForbiddenScope is set? > > Another option would be to post an async task to resolve/reject the promises. From the callstack as minimum in two places: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Conta... and https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/TreeS.... So I guess we cannot avoid forbidden scope since the tree gets modified. I put sheduleRejectPlayPromises in parch set 1 but as foolip pointed out it might be still forbidden at another moment as well.
Thanks for the review, will update and submit the final version.
Description was changed from
==========
Crash in blink::beforeCallEnteredCallback()
Crash occurs when HTMLMediaElement is moved to a new document
and play promise gets rejected because of the invoked load.
Check if the script is forbidden to schedule reject play promises.
BUG=676004
==========
to
==========
Crash in blink::beforeCallEnteredCallback()
Crash occurs because the script is forbidden due to the insertion
of the media element node when the media element is moved to a new document and
the existing play promise gets rejected because of the invoked load.
Check if the script is forbidden to skip reject play promises.
BUG=676004
==========
Description was changed from
==========
Crash in blink::beforeCallEnteredCallback()
Crash occurs because the script is forbidden due to the insertion
of the media element node when the media element is moved to a new document and
the existing play promise gets rejected because of the invoked load.
Check if the script is forbidden to skip reject play promises.
BUG=676004
==========
to
==========
Crash in blink::beforeCallEnteredCallback()
Crash occurs because the script is forbidden due to the insertion
of the media element node when the media element is moved to a new document
and the existing play promise gets rejected because of the invoked load.
Check if the script is forbidden to skip reject play promises.
BUG=676004
==========
The CQ bit was checked by a.obzhirov@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2594353003/#ps40001 (title: "Crash in blink::beforeCallEnteredCallback()")
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": 40001, "attempt_start_ts": 1484933556449700,
"parent_rev": "ee107748dd959954722d9c4cf56c3ce1988d2c7e", "commit_rev":
"87800a0c858e3ee1e2a406b3aae00e89ae7d077f"}
Message was sent while issue was closed.
Description was changed from
==========
Crash in blink::beforeCallEnteredCallback()
Crash occurs because the script is forbidden due to the insertion
of the media element node when the media element is moved to a new document
and the existing play promise gets rejected because of the invoked load.
Check if the script is forbidden to skip reject play promises.
BUG=676004
==========
to
==========
Crash in blink::beforeCallEnteredCallback()
Crash occurs because the script is forbidden due to the insertion
of the media element node when the media element is moved to a new document
and the existing play promise gets rejected because of the invoked load.
Check if the script is forbidden to skip reject play promises.
BUG=676004
Review-Url: https://codereview.chromium.org/2594353003
Cr-Commit-Position: refs/heads/master@{#445107}
Committed:
https://chromium.googlesource.com/chromium/src/+/87800a0c858e3ee1e2a406b3aae0...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/87800a0c858e3ee1e2a406b3aae0...
Message was sent while issue was closed.
Note that in the future you would want to postTask to resolve or reject the promise. Just dropping it on the floor like this patch does means the author gets no notification at all.
Message was sent while issue was closed.
haraken@chromium.org changed reviewers: + haraken@chromium.org
Message was sent while issue was closed.
(Sorry for the late reply -- it looks like I'm kicked out from the cc list.) https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:811: scheduleRejectPlayPromises(AbortError); On 2017/01/09 15:35:54, Anton Obzhirov wrote: > On 2017/01/04 15:49:00, foolip wrote: > > It's fairly likely that it's still forbidden in a moments time, I'd say forget > > the promises and leave the promises unresolved instead. > I see, will remove it. > scheduleRejectPlayPromises schedules a task on a new event loop, so ScriptForbiddenScope shouldn't be put on the call stack, right?
Message was sent while issue was closed.
On 2017/01/20 23:11:35, esprehn wrote: > Note that in the future you would want to postTask to resolve or reject the > promise. Just dropping it on the floor like this patch does means the author > gets no notification at all. What to do if scripts are still forbidden when the task runs? I'm not sure about this case, but for any promise that corresponds to an event that doesn't fire because the document the event target is in was destroyed, is there always an opportunity to reject the promise?
Message was sent while issue was closed.
On 2017/01/21 13:21:07, foolip_slow_very_sorry wrote: > On 2017/01/20 23:11:35, esprehn wrote: > > Note that in the future you would want to postTask to resolve or reject the > > promise. Just dropping it on the floor like this patch does means the author > > gets no notification at all. > > What to do if scripts are still forbidden when the task runs? I'm not sure about > this case, but for any promise that corresponds to an event that doesn't fire > because the document the event target is in was destroyed, is there always an > opportunity to reject the promise? Note that ScirptForbiddenScope is placed only on the stack. If you schedule an async task on a new event loop, the task shouldn't have a ScriptForbiddenScope.
Message was sent while issue was closed.
https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2594353003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:811: scheduleRejectPlayPromises(AbortError); On 2017/01/21 01:49:13, haraken wrote: > On 2017/01/09 15:35:54, Anton Obzhirov wrote: > > On 2017/01/04 15:49:00, foolip wrote: > > > It's fairly likely that it's still forbidden in a moments time, I'd say > forget > > > the promises and leave the promises unresolved instead. > > I see, will remove it. > > > > scheduleRejectPlayPromises schedules a task on a new event loop, so > ScriptForbiddenScope shouldn't be put on the call stack, right? > It seemed to work in this particular case, I mean ScriptForbiddenScope wasn't set when the task run HTMLMediaElement::rejectScheduledPlayPromises on next event loop.
Message was sent while issue was closed.
On 2017/01/21 13:22:58, haraken wrote: > On 2017/01/21 13:21:07, foolip_slow_very_sorry wrote: > > On 2017/01/20 23:11:35, esprehn wrote: > > > Note that in the future you would want to postTask to resolve or reject the > > > promise. Just dropping it on the floor like this patch does means the author > > > gets no notification at all. > > > > What to do if scripts are still forbidden when the task runs? I'm not sure > about > > this case, but for any promise that corresponds to an event that doesn't fire > > because the document the event target is in was destroyed, is there always an > > opportunity to reject the promise? > > Note that ScirptForbiddenScope is placed only on the stack. If you schedule an > async task on a new event loop, the task shouldn't have a ScriptForbiddenScope. I see, this is a different mechanism entirely than what makes events be dropped on the floor in inactive documents. Still it seems like in cases where the spec clearly defines the timing of when promises should be fulfilled/rejected, whether the promise is left hanging or settled later, the spec would have to do the same, i.e. we'd have to essentially have a spec-side counterpart to ScriptForbiddenScope. (Or does it already exist?)
Message was sent while issue was closed.
On 2017/01/21 21:01:36, foolip_slow_very_sorry wrote: > On 2017/01/21 13:22:58, haraken wrote: > > On 2017/01/21 13:21:07, foolip_slow_very_sorry wrote: > > > On 2017/01/20 23:11:35, esprehn wrote: > > > > Note that in the future you would want to postTask to resolve or reject > the > > > > promise. Just dropping it on the floor like this patch does means the > author > > > > gets no notification at all. > > > > > > What to do if scripts are still forbidden when the task runs? I'm not sure > > about > > > this case, but for any promise that corresponds to an event that doesn't > fire > > > because the document the event target is in was destroyed, is there always > an > > > opportunity to reject the promise? > > > > Note that ScirptForbiddenScope is placed only on the stack. If you schedule an > > async task on a new event loop, the task shouldn't have a > ScriptForbiddenScope. > > I see, this is a different mechanism entirely than what makes events be dropped > on the floor in inactive documents. > > Still it seems like in cases where the spec clearly defines the timing of when > promises should be fulfilled/rejected, whether the promise is left hanging or > settled later, the spec would have to do the same, i.e. we'd have to essentially > have a spec-side counterpart to ScriptForbiddenScope. (Or does it already > exist?) As far as I know, the spec doesn't have the notion. ScriptForbiddenScope is a notion Blink has introduced to the code base to prevent user scripts from getting triggered in unsafe places. In this specific case, the promise is being rejected (and any arbitrary user script is going to run) while a Node is being inserted to the DOM tree. This is unsafe and that's why we're hitting the ScriptForbiddenScope assert. Even if the spec doesn't have a notion of ScriptForbiddenScope, if it has a call path that rejects promises in such unsafe places, that's likely to be a bug of the spec. (IMHO it would be helpful to introduce the notion of ScriptForbiddenScope to the spec.)
Message was sent while issue was closed.
On 2017/01/23 00:04:45, haraken wrote: > On 2017/01/21 21:01:36, foolip_slow_very_sorry wrote: > > On 2017/01/21 13:22:58, haraken wrote: > > > On 2017/01/21 13:21:07, foolip_slow_very_sorry wrote: > > > > On 2017/01/20 23:11:35, esprehn wrote: > > > > > Note that in the future you would want to postTask to resolve or reject > > the > > > > > promise. Just dropping it on the floor like this patch does means the > > author > > > > > gets no notification at all. > > > > > > > > What to do if scripts are still forbidden when the task runs? I'm not sure > > > about > > > > this case, but for any promise that corresponds to an event that doesn't > > fire > > > > because the document the event target is in was destroyed, is there always > > an > > > > opportunity to reject the promise? > > > > > > Note that ScirptForbiddenScope is placed only on the stack. If you schedule > an > > > async task on a new event loop, the task shouldn't have a > > ScriptForbiddenScope. > > > > I see, this is a different mechanism entirely than what makes events be > dropped > > on the floor in inactive documents. > > > > Still it seems like in cases where the spec clearly defines the timing of when > > promises should be fulfilled/rejected, whether the promise is left hanging or > > settled later, the spec would have to do the same, i.e. we'd have to > essentially > > have a spec-side counterpart to ScriptForbiddenScope. (Or does it already > > exist?) > > As far as I know, the spec doesn't have the notion. ScriptForbiddenScope is a > notion Blink has introduced to the code base to prevent user scripts from > getting triggered in unsafe places. > > In this specific case, the promise is being rejected (and any arbitrary user > script is going to run) while a Node is being inserted to the DOM tree. This is > unsafe and that's why we're hitting the ScriptForbiddenScope assert. Even if the > spec doesn't have a notion of ScriptForbiddenScope, if it has a call path that > rejects promises in such unsafe places, that's likely to be a bug of the spec. > > (IMHO it would be helpful to introduce the notion of ScriptForbiddenScope to the > spec.) If you have a good idea of what that would entail, can you file an issue at https://github.com/whatwg/html/issues/new? Perhaps domenic@ would have some ideas for this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
