Closed
Bug 1390076
Opened 7 years ago
Closed 7 years ago
Label MainThreadInvoker
Categories
(Core :: Disability Access APIs, enhancement, P1)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
Details
Attachments
(1 file)
1.58 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
This runnable is shown up recently in the unlabeled list with high frequency https://gist.github.com/bevis-tseng/12cf108f681467ca7d8f3088db4eb584
Updated•7 years ago
|
Priority: -- → P1
Comment 1•7 years ago
|
||
Bill, it looks like you added this...does this runnable touch content in any way?
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 2•7 years ago
|
||
Not really sure what this is for but the result looks fine if dispatched with SystemGroup: https://treeherder.mozilla.org/#/jobs?repo=try&revision=402d2bfca693ab075d4f198f7166c7e57df4e791
Assignee | ||
Comment 3•7 years ago
|
||
After further review about the use of MainThreadInvoker::Invoke(), I found that most the call sites are about invoking some MS-COM getters such as IUnknown::QueryInterface()/IDispatch::{GetTypeInfo|GetIDsOfNames} which unlike to be something related to a JS callback. The only concern is the ICallFrame::Invoke() in MainThreadHandOff.cpp which could be any possible COM interface to be invoked on the main thread, IIUC: http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/ipc/mscom/MainThreadHandoff.cpp#141 Hi :aklotz, Could you tell us more about what MainThreadInvoker::Invoke() could touch? Will a JS callback be called implicitly from ICallFrame::Invoke()? If yes, would you mind to point out the call path as an example? (This will help us to associate proper TabGroup for dispatching the SyncRunnable used by MainThreadInvoker.) If no, then we are safe to dispatch the SyncRunnable using SystemGroup. Here is some background about the labeling in Quantum-DOM Project: https://wiki.mozilla.org/Quantum/DOM#Runnable_grouping
Component: DOM → Disability Access APIs
Flags: needinfo?(aklotz)
Sorry, I don't know anything about this.
Flags: needinfo?(wmccloskey)
Comment 5•7 years ago
|
||
(In reply to Bevis Tseng[:bevis][:btseng] from comment #3) > Will a JS callback be called implicitly from ICallFrame::Invoke()? No. Currently it is only used to call back into C++ a11y code. > If no, then we are safe to dispatch the SyncRunnable using SystemGroup. Based on what I read on the wiki page (thanks!), that should be safe.
Flags: needinfo?(aklotz)
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #5) > (In reply to Bevis Tseng[:bevis][:btseng] from comment #3) > > Will a JS callback be called implicitly from ICallFrame::Invoke()? > > No. Currently it is only used to call back into C++ a11y code. > > > If no, then we are safe to dispatch the SyncRunnable using SystemGroup. > > Based on what I read on the wiki page (thanks!), that should be safe. Thanks for your clarification!
Comment 8•7 years ago
|
||
I am holding off on r+ until I can discuss this a bit more with some a11y people. Be assured that I not ignoring your review request!
Comment 10•7 years ago
|
||
Comment on attachment 8903006 [details] [diff] [review] (v1) Patch: Label MainThreadInvoker using SystemGroup. Review of attachment 8903006 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/mscom/MainThreadInvoker.cpp @@ +11,5 @@ > #include "mozilla/Assertions.h" > #include "mozilla/ClearOnShutdown.h" > #include "mozilla/DebugOnly.h" > #include "mozilla/HangMonitor.h" > +#include "mozilla/SystemGroup.h" Nit: please move this line to after the include for RefPtr.h to preserve alphabetical ordering.
Attachment #8903006 -
Flags: review?(aklotz) → review+
Updated•7 years ago
|
Flags: needinfo?(eitan)
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #10) > Nit: please move this line to after the include for RefPtr.h to preserve > alphabetical ordering. Will do!
Comment 12•7 years ago
|
||
Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/75b91420fff7 Label MainThreadInvoker using SystemGroup. r=aklotz
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/75b91420fff7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•