Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bevis, Assigned: bevis)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

This runnable is shown up recently in the unlabeled list with high frequency
https://gist.github.com/bevis-tseng/12cf108f681467ca7d8f3088db4eb584
Priority: -- → P1
Bill, it looks like you added this...does this runnable touch content in any way?
Flags: needinfo?(wmccloskey)
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
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)
(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)
(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!
Treeherder results on windows builds in comment 2 looks fine.
Assignee: nobody → btseng
Status: NEW → ASSIGNED
Attachment #8903006 - Flags: review?(aklotz)
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!
Eitan can you help Aaron here?
Flags: needinfo?(eitan)
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+
Flags: needinfo?(eitan)
(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!
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b91420fff7
Label MainThreadInvoker using SystemGroup. r=aklotz
https://hg.mozilla.org/mozilla-central/rev/75b91420fff7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.