Summary: | [Mac] media/track/audio-track.html is flakey | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca, ap, commit-queue, eric.carlson, esprehn+autocc, kangil.han, sam | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jer Noble
2014-11-04 18:04:41 PST
Created attachment 241010 [details]
Patch
Anders, is there an existing message queue class that should be used instead? I feel like we have too many. MessageQueue? Document::postTask? (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. (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. > 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.
(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. (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> (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. Created attachment 241308 [details]
Patch
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. 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. Committed r176024: <http://trac.webkit.org/changeset/176024> |