Bug 138394 – [Mac] media/track/audio-track.html is flakey

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

[Mac] media/track/audio-track.html is flakey

[Jer Noble]

Created attachment 241010 [details]
Patch

[Alexey Proskuryakov]

Anders, is there an existing message queue class that should be used instead? I feel like we have too many. MessageQueue? Document::postTask?

[Jer Noble]

(In reply to comment #2)
> Anders, is there an existing message queue class that should be used
> instead? I feel like we have too many. MessageQueue?

MessageQueue isn't really for Events, but more for cross-thread messaging.

> Document::postTask?

I don't think we should re-use Document::postTask() for this. It does not guarantee ordering across ScriptExecutionContexts.

Ideally, Sam's work to add official task queues <http://www.w3.org/TR/html5/webappapis.html#task-queue> would add a specific Event task queue, which would replace GenericEventQueue, and would be tied to a Browsing Context.

I can add a FIXME to this effect.

[Jer Noble]

(In reply to comment #3)
> Ideally, Sam's work to add official task queues
> <http://www.w3.org/TR/html5/webappapis.html#task-queue> would add a specific
> Event task queue, which would replace GenericEventQueue, and would be tied
> to a Browsing Context.

In fact, GenericEventQueue could become the basis for the Event Task Queue.

[Alexey Proskuryakov]

> I don't think we should re-use Document::postTask() for this. It does not guarantee ordering across ScriptExecutionContexts.

This actually sounds like a good thing - documents are isolated, and adding a shared event queue would be a side channel for XSS. It obviously wouldn't be the biggest one, so it's not a show stopper in any way, however it surprises me if that's a desirable behavior.

[Jer Noble]

(In reply to comment #5)
> > I don't think we should re-use Document::postTask() for this. It does not guarantee ordering across ScriptExecutionContexts.
> 
> This actually sounds like a good thing - documents are isolated, and adding
> a shared event queue would be a side channel for XSS. It obviously wouldn't
> be the biggest one, so it's not a show stopper in any way, however it
> surprises me if that's a desirable behavior.

Hm, maybe.

[Jer Noble]

(In reply to comment #6)
> (In reply to comment #5)
> > > I don't think we should re-use Document::postTask() for this. It does not guarantee ordering across ScriptExecutionContexts.
> > 
> > This actually sounds like a good thing - documents are isolated, and adding
> > a shared event queue would be a side channel for XSS. It obviously wouldn't
> > be the biggest one, so it's not a show stopper in any way, however it
> > surprises me if that's a desirable behavior.
> 
> Hm, maybe.

Actually, the spec specifies a minimum of one event loop (and Event task queue) per User Agent, and _allows_ for one event loop per Browsing Context, but does not require it.

So it may be a good idea to segregate event loops on a Browsing Context (i.e. Document) basis, it's not necessary according to the spec.

<http://www.w3.org/TR/html5/webappapis.html#event-loops>

[Jer Noble]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > > I don't think we should re-use Document::postTask() for this. It does not guarantee ordering across ScriptExecutionContexts.
> > > 
> > > This actually sounds like a good thing - documents are isolated, and adding
> > > a shared event queue would be a side channel for XSS. It obviously wouldn't
> > > be the biggest one, so it's not a show stopper in any way, however it
> > > surprises me if that's a desirable behavior.
> > 
> > Hm, maybe.
> 
> Actually, the spec specifies a minimum of one event loop (and Event task
> queue) per User Agent, and _allows_ for one event loop per Browsing Context,
> but does not require it.
> 
> So it may be a good idea to segregate event loops on a Browsing Context
> (i.e. Document) basis, it's not necessary according to the spec.
> 
> <http://www.w3.org/TR/html5/webappapis.html#event-loops>

And, under a close reading of the spec, at a minimum Events must be dispatched in enqueued order across same-origin Browsing Contexts.

[Jer Noble]

Created attachment 241308 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 241308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241308&action=review

> Source/WebCore/dom/GenericEventQueue.cpp:33
> +#include <wtf/text/CString.h>

Is this include needed?

> Source/WebCore/dom/GenericEventQueue.cpp:66
> +    static NeverDestroyed<Timer> timer(GenericEventQueue::sharedTimerFired);

Please add an ASSERT(isMainThread()) here. The last thing we need is for this to get used from a secondary thread.

> Source/WebCore/dom/GenericEventQueue.cpp:88
> +    static NeverDestroyed<Deque<WeakPtr<GenericEventQueue>>> queues;

Please add an ASSERT(isMainThread()) here, too.

[Jer Noble]

Comment on attachment 241308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241308&action=review

>> Source/WebCore/dom/GenericEventQueue.cpp:33
>> +#include <wtf/text/CString.h>
> 
> Is this include needed?

No, it's not. I had logging in this file previously, but removed it before uploading. I'll remove this include as well.

>> Source/WebCore/dom/GenericEventQueue.cpp:66
>> +    static NeverDestroyed<Timer> timer(GenericEventQueue::sharedTimerFired);
> 
> Please add an ASSERT(isMainThread()) here. The last thing we need is for this to get used from a secondary thread.

Ok.

>> Source/WebCore/dom/GenericEventQueue.cpp:88
>> +    static NeverDestroyed<Deque<WeakPtr<GenericEventQueue>>> queues;
> 
> Please add an ASSERT(isMainThread()) here, too.

Ok.

[Jer Noble]

Committed r176024: <http://trac.webkit.org/changeset/176024>