Closed
Bug 1409985
Opened 7 years ago
Closed 7 years ago
Don't fire DOMEvent during StableState
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
Details
Attachments
(1 file, 2 obsolete files)
7.29 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
This is to address the issue found in bug 1193394 comment 44.
The "Stable" state is used to run some task not visible from the script.
However, there are 2 use cases that breaks this rule:
1. HTMLMediaElement::RunInStableState() -> MediaDecoder::BackgroundVideoDecodingPermissionObserver::EnableEvent():
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bee6bf1006e7627557053e94dfca13f4754b0b5&selectedJob=137781048
2. MediaStreamGraphImpl::RunInStableState() -> SpeechSynthesisUtterance::DispatchSpeechSynthesisEvent():
https://public-artifacts.taskcluster.net/MimKd4uhQfmBv6NHDVS3Mg/0/public/logs/live_backing.log
We should eliminate these abuse problem and add assertion to identify this problem in the future.
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•7 years ago
|
||
1. Add assertion to prevent dispatching non-chrome event in stable states.
2. Use AsyncEventDispatcher in stable states.
Attachment #8920454 -
Flags: review?(bugs)
Assignee | ||
Comment 2•7 years ago
|
||
Treeherder result looks good, btw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f10d6d561f1cbb256de309f744afab324c14bdd1
Assignee | ||
Comment 3•7 years ago
|
||
Comment on attachment 8920454 [details] [diff] [review]
(v1) Patch: Bug 1409985 - Don't fire DOMEvent during StableState.
Review of attachment 8920454 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/events/EventDispatcher.cpp
@@ +604,5 @@
>
> + // Events shall not be fired to content while we are in stable state.
> + MOZ_ASSERT_IF(!aEvent->mFlags.mOnlyChromeDispatch,
> + CycleCollectedJSContext::Get() &&
> + !CycleCollectedJSContext::Get()->IsDoingStableStates());
I should add nsContentUtils::IsDoingStableStates() to simplify the call of
MOZ_ASSERT(CycleCollectedJSContext::Get(), "Must be on a script thread!");
MOZ_ASSERT(CycleCollectedJSContext::Get()->IsDoingStableStates());
Comment 4•7 years ago
|
||
Why non-chrome event? All the events should be prevented.
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #4)
> Why non-chrome event? All the events should be prevented.
I thought it's fine for chrome event per offline discussion and to keep HTMLMediaElement unchanged.
I could try AsyncEventDispatcher for HTMLMediaElement to see if there is anything wrong.
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8920454 [details] [diff] [review]
(v1) Patch: Bug 1409985 - Don't fire DOMEvent during StableState.
cancel the review per comment 5.
Attachment #8920454 -
Flags: review?(bugs)
Comment 7•7 years ago
|
||
No JS should run during stable state, IMO. Well, possibly some JS implemented XPCOM components.
Assignee | ||
Comment 8•7 years ago
|
||
1. Dispatch async task for the following events which could be fired during stable state:
a. MediaDecoder::BackgroundVideoDecodingPermissionObserver::EnableEvent()
b. MediaDecoder::BackgroundVideoDecodingPermissionObserver::DisableEvent()
c. SpeechSynthesisUtterance::DispatchSpeechSynthesisEvent()
2. Add assertion in EventDispatcher to catch the abuse of RunInStableState().
Treeherder result looks fine, btw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38d79c55603ae46d3b9b930e5657b7b8a8f309e6&group_state=expanded
Attachment #8920454 -
Attachment is obsolete: true
Attachment #8921379 -
Flags: review?(bugs)
Comment 9•7 years ago
|
||
Comment on attachment 8921379 [details] [diff] [review]
(v2) Patch: Don't fire DOMEvent during StableState.
># HG changeset patch
># User Bevis Tseng <btseng@mozilla.com>
># Date 1508408431 -28800
># Thu Oct 19 18:20:31 2017 +0800
># Node ID 36713cb4d1b4e70e8015780c07cbc88520cf7ce7
># Parent 9056f2ee492fa481aa86146aba236c074628e9fd
>Bug 1409985 - Don't fire DOMEvent during StableState. r=smaug
>
>diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
>--- a/dom/base/nsContentUtils.cpp
>+++ b/dom/base/nsContentUtils.cpp
>@@ -5831,6 +5831,14 @@ nsContentUtils::RunInMetastableState(alr
> }
>
> /* static */
>+bool
>+nsContentUtils::IsDoingStableStates()
Could it be called IsInStableOrMetaStableState(), here and elsewhere.
>+ if (nsCOMPtr<nsIDocument> doc = GetOwnerDoc()) {
>+ doc->Dispatch(TaskCategory::Other,
>+ NewRunnableMethod(
>+ "MediaDecoder::BackgroundVideoDecodingPermissionObserver::EnableEvent",
>+ this, &MediaDecoder::BackgroundVideoDecodingPermissionObserver::EnableEvent));
Nit, use 2 spaces for indentation
>+ if (nsCOMPtr<nsIDocument> doc = GetOwnerDoc()) {
>+ doc->Dispatch(TaskCategory::Other,
>+ NewRunnableMethod(
>+ "MediaDecoder::BackgroundVideoDecodingPermissionObserver::DisableEvent",
>+ this, &MediaDecoder::BackgroundVideoDecodingPermissionObserver::DisableEvent));
Nit, 2 spaces for indentation
>@@ -174,7 +176,16 @@ SpeechSynthesisUtterance::DispatchSpeech
>
> RefPtr<SpeechSynthesisEvent> event =
> SpeechSynthesisEvent::Constructor(this, aEventType, init);
>- DispatchTrustedEvent(event);
>+
>+ if (nsContentUtils::IsDoingStableStates()) {
>+ // Events shall not be fired synchronously to prevent anything visible
>+ // from the scripts while we are in stable state.
>+ RefPtr<AsyncEventDispatcher> asyncDispatcher =
>+ new AsyncEventDispatcher(this, event);
>+ asyncDispatcher->PostDOMEvent();
>+ } else {
>+ DispatchTrustedEvent(event);
>+ }
Could you explain why this approach, and why not make the MediaStreamcallback thingie to call speech API asynchronously?
(I thought we chatted about that on IRC)
I think I'd prefer that. But if there is some reason for this approach, explain and ask review again.
Attachment #8921379 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 10•7 years ago
|
||
1. Rename IsDoingStableStates() to IsInStableOrMetaStableState()
2. Call SynthStreamListener::DoNotify{Started|Ended} asynchronously after stream state update.
(In previous patch, besides event dispatching, I thought that there are other stuff in DoNotifyStarted/Ended was expected to be done during stable state, so I only move the event dispatching part to a new task. However, after testing with this latest patch to async call DoNotifyStarted/Ended entirely, everything looks fine on try [1] as well, so I update this one for the review instead.)
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ce4420bbdd4326a7362e7389f93392bba76e086
Attachment #8921379 -
Attachment is obsolete: true
Attachment #8922618 -
Flags: review?(bugs)
Updated•7 years ago
|
Attachment #8922618 -
Flags: review?(bugs) → review+
Comment 11•7 years ago
|
||
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22003af130ed
Don't fire DOMEvent during StableState. r=smaug
Comment 12•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•