1395208 - Make RefreshDriverVsyncObserver::NormalPriorityNotify implementing nsILabelableRunnable

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect, P2)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

Similar to vsync runnable, this runnable could affect multiple foreground tabs at the same time. Implementing nsILabelableRunnable allows scheduler to know which scheduler groups are related.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Unfortunately, the test result looks NG after rewriting with nsILabelableRunnable.
We hit a lot of "Assertion failure: IsSafeToRun()" or "Assertion failure: !group->IsRunning()":
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4972c4da1733a3128ceef434ba779720843476f5

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevis][:btseng] from comment #1)
> Unfortunately, the test result looks NG after rewriting with
> nsILabelableRunnable.
> We hit a lot of "Assertion failure: IsSafeToRun()" or "Assertion failure:
> !group->IsRunning()":
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4972c4da1733a3128ceef434ba779720843476f5

After further investigation of "Assertion failure:> !group->IsRunning()",
I found that sometimes dom::TabChild::GetActiveTabs() contains redundant entries [1] which causes Scheduler to call SchedulerGroup::SetIsRunning() multiple times [2].

[1] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/dom/ipc/TabChild.cpp#2593
[2] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/xpcom/threads/Scheduler.cpp#569

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevis][:btseng] from comment #2)
> After further investigation of "Assertion failure:> !group->IsRunning()",
> I found that sometimes dom::TabChild::GetActiveTabs() contains redundant
> entries [1] which causes Scheduler to call SchedulerGroup::SetIsRunning()
> multiple times [2].
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 51eae084534f522a502e4b808484cde8591cb502/dom/ipc/TabChild.cpp#2593
> [2]
> http://searchfox.org/mozilla-central/rev/
> 51eae084534f522a502e4b808484cde8591cb502/xpcom/threads/Scheduler.cpp#569

In addition, sometimes we got the same SchedulerGroup from 2 different TabChilds which could also hit the same assertion even if we prevent redundant entries in TabChild::sActiveTabs.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Note: docshell/test/navigation/test_sibling-off-domain.html is the test where I saw the symptoms mentioned comment 2 and comment 3. (Not 100% reproducible)

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

The failure of "Assertion failure:> !group->IsRunning()" are fixed in latest try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2761df81f057fa51fc3c0221941f849501e86380
There are 2 assertions remained to be fixed:
1. Assertion failure: IsSafeToRun(), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/SchedulerGroup.h:75
2. Assertion failure: !cx->enableAccessValidation || cx->compartment()->isAccessValid(), at /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:397

Maybe we should move the fix of part 2 to a new bug which is obvious something wrong in the implementation of TabChild::sActiveTabs and EventLoopActivation::mEventGroups.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (WIP) Patch: Make RefreshDriverVsyncObserver::NormalPriorityNotify implementing nsILabelableRunnable.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Reset assignee because not actively working on this bug.