stylo: Enable stylo-chrome by default

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
10 days ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Depends on 2 bugs, {feature})

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 60+, firefox-esr52 unaffected, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

No description provided.
Assignee: nobody → xidorn+moz
Depends on: 1417207
Depends on: 1417281
Depends on: 1417548
Depends on: 1417563
No longer depends on: 1417563
Depends on: 1417600
Although I have pushed a try push for talos before, but there seems to be some issue with the tests, and some of them may be fixed by some patches landed. I'm going to do another talos push once bug 1417563 gets fixed.
Depends on: 1418082
Priority: -- → P3
Hmmm... not sure what's happening to browser/components/contextualidentity/test/browser/browser_aboutURLs.js :/
Depends on: 1419903
Depends on: 1419943
Depends on: 1419554
talos tests are still running. I've seen some big regression and some (relatively smaller) big improvement. Not sure what should we do to mitigate the regressions.
Those regressions seem bad enough that I think we should try to resolve them before flipping the pref.
(In reply to Cameron McCormack (:heycam) from comment #9)
> Those regressions seem bad enough that I think we should try to resolve them
> before flipping the pref.

Yeah it really worries me a lot. Probably need to see why's that. Not sure what's the best way to do the investigation.
I think what happens is O(n^2) behavior when resolving XBL bindings. I'm looking into it.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> I think what happens is O(n^2) behavior when resolving XBL bindings. I'm
> looking into it.

When resolving nested XBL bindings, I mean.
Depends on: 1420423
Flags: needinfo?(emilio)
Depends on: 1421374
Depends on: 1436606
Depends on: 1437774
Depends on: 1437790
Depends on: 1437796
Depends on: 1438078
Attachment #8931481 - Attachment is obsolete: true
Attachment #8931481 - Flags: review?(cam)
Attachment #8931482 - Attachment is obsolete: true
Attachment #8931482 - Flags: review?(cam)
Comment on attachment 8951223 [details]
Bug 1417138 part 1 - Skip about:downloads in about url test.

https://reviewboard.mozilla.org/r/220488/#review226428

r=me . For bug 1419943, might be worth trying to repro locally passing the --verify flag to mochitest.
Attachment #8951223 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8931483 [details]
Bug 1417138 part 2 - Enable stylo-chrome by default.

https://reviewboard.mozilla.org/r/202576/#review226584
Attachment #8931483 - Flags: review?(bobbyholley) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92fa88d94170
part 1 - Skip about:downloads in about url test. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/4e0d4c91940b
part 2 - Enable stylo-chrome by default. r=bholley
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd4694fdd87b
followup - Update reftest manifest for stylo-chrome.
Backout by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0854548560aa
Backed out changeset dd4694fdd87b followup- backout for failing in slave/test/build/tests/reftest/tests/layout/reftests/xul/menulist-shrinkwrap-2.xul r=xidorn  on a CLOSED TREE
Flags: needinfo?(xidorn+moz)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c559b013b025
part 1 - Skip about:downloads in about url test. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/0264b78fe72b
part 2 - Enable stylo-chrome by default. r=bholley
Backed for mochitest failures on Android dom/indexedDB/test/test_message_manager_ipc.html 

backout : https://hg.mozilla.org/integration/autoland/rev/aa5d85f2a7ec876d9d35f4123445b8e1e9344b61
push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0264b78fe72bf4b780ac74afd541f48fe30eac09&filter-searchStr=android%20debug%20m(15)&selectedJob=162657489
failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162657489&repo=autoland&lineNumber=1661

[task 2018-02-16T15:08:43.797Z] 15:08:43     INFO -  123 INFO None124 INFO TEST-START | dom/indexedDB/test/test_message_manager_ipc.html
[task 2018-02-16T15:14:01.486Z] 15:14:01     INFO -  Buffered messages logged at 15:08:44
[task 2018-02-16T15:14:01.486Z] 15:14:01     INFO -  125 INFO Got load event
[task 2018-02-16T15:14:01.486Z] 15:14:01     INFO -  126 INFO Prefs set
[task 2018-02-16T15:14:01.486Z] 15:14:01     INFO -  Buffered messages logged at 15:08:45
[task 2018-02-16T15:14:01.487Z] 15:14:01     INFO -  127 INFO Permissions set
[task 2018-02-16T15:14:01.487Z] 15:14:01     INFO -  Buffered messages finished
[task 2018-02-16T15:14:01.487Z] 15:14:01     INFO -  128 INFO TEST-UNEXPECTED-FAIL | dom/indexedDB/test/test_message_manager_ipc.html | Test timed out.
[task 2018-02-16T15:14:01.487Z] 15:14:01     INFO -      reportError@SimpleTest/TestRunner.js:121:7
[task 2018-02-16T15:14:01.487Z] 15:14:01     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
[task 2018-02-16T15:14:01.487Z] 15:14:01     INFO -  129 INFO TEST-UNEXPECTED-ERROR | dom/indexedDB/test/test_message_manager_ipc.html | unexpected-crash-dump-found - This test left crash dumps behind, but we weren't expecting it to!
[task 2018-02-16T15:14:01.487Z] 15:14:01     INFO -  130 INFO Found unexpected crash dump file /storage/sdcard/tests/profile/minidumps/6c3c6d10-459f-e57f-86bd-4a426fee1525.dmp.
[task 2018-02-16T15:14:01.488Z] 15:14:01     INFO -  131 INFO Found unexpected crash dump file /storage/sdcard/tests/profile/minidumps/6c3c6d10-459f-e57f-86bd-4a426fee1525.extra.
[task 2018-02-16T15:14:01.488Z] 15:14:01     INFO -  132 INFO TEST-UNEXPECTED-CRASH | dom/indexedDB/test/test_message_manager_ipc.html | Finished in 315092ms
[task 2018-02-16T15:14:01.488Z] 15:14:01     INFO -  {u'runtime': 315092}
[task 2018-02-16T15:14:01.488Z] 15:14:01     INFO -  TEST-INFO took 315076ms
Flags: needinfo?(xidorn+moz)
The previous failures seem to be unrelated to the failure caused by this push, actually.

It seems there are some crashes for IPC happens on Android with stylo-chrome enabled... But the log doesn't really include much useful information :/
Kato-san, do you have any suggestion about how can we fix that crash?
Flags: needinfo?(xidorn+moz) → needinfo?(m_kato)
Not intended to remove my own ni?...
Flags: needinfo?(xidorn+moz)
So the full list of crashing tests are:
* dom/file/tests/test_ipc_messagemanager_blob.html
* devtools/shared/heapsnapshot/tests/mochitest/test_saveHeapSnapshot_e10s_01.html
* dom/indexedDB/test/test_message_manager_ipc.html
* dom/ipc/tests/test_blob_sliced_from_child_process.html
* dom/presentation/tests/mochitest/test_presentation_receiver_auxiliary_navigation_oop.html
* dom/browser-element/mochitest/test_browserElement_inproc_PurgeHistory.html
* dom/browser-element/mochitest/test_browserElement_inproc_Stop.html

All of the tests above involve <iframe mozbrowser>, while there are tests which also use that but not affected.
I downloaded two dump files, and extract the memory from the crashing thread.

Both memory content starts with builds/worker/workspace/build/src/modules/libpref/Preferences.cpp:886, which points to https://searchfox.org/mozilla-central/rev/cac28623a15ace458a8f4526e107a71db1519daf/modules/libpref/Preferences.cpp#885-886

That make me wonder where is the crash message goes... and it seems for Android, it goes to logcat. And in log from logcat, I see this line:
> MOZ_CRASH: Hit MOZ_CRASH(accessing non-early pref consoleservice.logcat before late prefs are set) at /builds/worker/workspace/build/src/modules/libpref/Preferences.cpp:886
which seems to be the reason of this crash...
So consoleservice.logcat is read by AddConsolePrefWatchers [1] which is dispatched to the main thread [2] when nsConsoleService is initialized.

Looking around for when that service is used, I suspect that this is related to the error reporter. Maybe this is triggered by CSS errors gets reporting?

It is not clear to me how Fennec works in general. Code in [2] indicates that the read should happen in the main thread, however the crashing thread is Thread 10. Also, it seems when the crash happens, the process should have run for a while, why are the prefs not set at that moment? Maybe we are spawning new process for mozbrowser? In that case, shouldn't the main thread be Thread 0 in the new process?

Also this doesn't explain why this happens when stylo-chrome is enabled, since if there is error to be reported while prefs are not set yet, it should crash the old style system as well...


[1] https://searchfox.org/mozilla-central/rev/cac28623a15ace458a8f4526e107a71db1519daf/xpcom/base/nsConsoleService.cpp#154
[2] https://searchfox.org/mozilla-central/rev/cac28623a15ace458a8f4526e107a71db1519daf/xpcom/base/nsConsoleService.cpp#181
It seems this specific case indeed happened before as shown in bug 1430500. Maybe stylo-chrome makes it happen more reliably?
Depends on: 1430500
Ah... that's not very useful actually... AddConsolePrefWatchers is the only place which queries consoleservice.logcat, and AddConsolePrefWatchers is dispatched by nsConsoleService::Init(). So I guess the real question would be why nsConsoleService::Init is called at some point when prefs is not ready.

Probably I can do a try push moving the preference query to nsConsoleService::Init().
I moved the preference query to nsConsoleService::Init(), and got a stack which looks more helpful:
Thread 10 (crashed)
 0  libmozglue.so!MOZ_CrashPrintf [Assertions.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 63 + 0x0]
 1  libxul.so!pref_HashTableLookupInner [Preferences.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 885 + 0xf]
 2  libxul.so!pref_HashTableLookup [Preferences.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 897 + 0x3]
 3  libxul.so!mozilla::Preferences::GetBool [Preferences.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 3931 + 0x5]
 4  libxul.so!mozilla::Preferences::GetBool [Preferences.h:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 140 + 0x3]
 5  libxul.so!mozilla::Preferences::AddBoolVarCache [Preferences.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 4430 + 0xd]
 6  libxul.so!nsConsoleService::Init [nsConsoleService.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 172 + 0xb]
 7  libxul.so!nsConsoleServiceConstructor [XPCOMInit.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 207 + 0x9]
 8  libxul.so!nsComponentManagerImpl::CreateInstanceByContractID [nsComponentManager.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 1086 + 0xb]
 9  libxul.so!nsComponentManagerImpl::GetServiceByContractID [nsComponentManager.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 1446 + 0xd]
10  libxul.so!nsGetServiceByContractID::operator() [nsComponentManagerUtils.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 280 + 0x3]
11  libxul.so!nsCOMPtr<mozIPersonalDictionary>::assign_from_gs_contractid [nsCOMPtr.h:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 1215 + 0x7]
12  libxul.so!nsCOMPtr<nsIConsoleService>::nsCOMPtr [nsCOMPtr.h:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 577 + 0x5]
13  libxul.so!LogMessage [ManifestParser.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 151 + 0x9]
14  libxul.so!nsComponentManagerImpl::RegisterCIDEntryLocked [nsComponentManager.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 487 + 0x5]
15  libxul.so!nsComponentManagerImpl::RegisterModule [nsComponentManager.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 440 + 0x7]
16  libxul.so!nsComponentManagerImpl::Init [nsComponentManager.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 308 + 0x7]
17  libxul.so!NS_InitXPCOM2 [XPCOMInit.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 676 + 0x3]
18  libxul.so!XRE_InitEmbedding2 [nsEmbedFunctions.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 190 + 0x5]
19  libxul.so!mozilla::ipc::ScopedXREEmbed::Start [ScopedXREEmbed.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 97 + 0x3]
20  libxul.so!mozilla::dom::ContentProcess::Init [ContentProcess.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 233 + 0x5]
21  libxul.so!XRE_InitChildProcess [nsEmbedFunctions.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 694 + 0x9]
22  libmozglue.so!Java_org_mozilla_gecko_mozglue_GeckoLoader_nativeRun [APKOpen.cpp:1d35c865b1febf3426b2e213063fcd0a432a4ad4 : 430 + 0xb]
23  libdvm.so + 0x1dc4e

(from https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d35c865b1febf3426b2e213063fcd0a432a4ad4)

So from the stack, it seems nsComponentManagerImpl::RegisterCIDEntryLocked logs something which triggers the construction of nsConsoleService. And the log that function does is [1]
> "While registering XPCOM module %s, trying to re-register CID '%s' already registered by %s."

It's... completely unclear to me how this issue is triggered by enabling stylo-chrome...

[1] https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/xpcom/components/nsComponentManager.cpp#487-490
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #37)
> So from the stack, it seems nsComponentManagerImpl::RegisterCIDEntryLocked
> logs something which triggers the construction of nsConsoleService. And the
> log that function does is [1]
> > "While registering XPCOM module %s, trying to re-register CID '%s' already registered by %s."
> 
> It's... completely unclear to me how this issue is triggered by enabling
> stylo-chrome...

In stylo-chrome, are some CIDs perhaps no longer registered that were registered before? Or are they registered sooner/later because other stuff is registered first, making the race condition happen more reliably?
The message is
> While registering XPCOM module <static module>, trying to re-register CID '{c401eb80-f9ea-11d3-bb6f-e732b73ebe7c}' already registered by <static module>.

The CID seems to belong to screen manager, [1] but it's possible that it just happens to be that module.

[1] https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/widget/nsWidgetsCID.h#124-125
(In reply to :Gijs from comment #38)
> In stylo-chrome, are some CIDs perhaps no longer registered that were
> registered before? Or are they registered sooner/later because other stuff
> is registered first, making the race condition happen more reliably?

I... don't think stylo-chrome touches anything in the startup path before any system principal document starts loading.
In a normal build, the following message can also been seen from logcat for the affected tests:
> [Child 1911, Main Thread] WARNING: Re-registering a CID?: file /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp, line 475
> [Child 1911, Main Thread] WARNING: Re-registering a CID?: file /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp, line 475
> [Child 1911, Main Thread] WARNING: Cannot create startup observer : service,@mozilla.org/browser/browser-clh;1: file /builds/worker/workspace/build/src/toolkit/xre/nsAppStartupNotifier.cpp, line 78
> [Child 1911, Main Thread] ###!!! ASSERTION: Startup Observer failed!
> : 'NS_SUCCEEDED(rv)', file /builds/worker/workspace/build/src/toolkit/xre/nsAppStartupNotifier.cpp, line 71
which indicates that the re-registering thing is not specific to stylo-chrome.

The question is, then, why later LogMessage doesn't take effect? e.g. either crash for querying preference or output the message to logcat? It seems to me that the message should go to logcat with the default settings.
Depends on: 1439406
Anyway, it seems bug 1439406 should fix that issue.
Flags: needinfo?(xidorn+moz)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f08bf9a7a50
part 1 - Skip about:downloads in about url test. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/7a63a457c8e8
part 2 - Enable stylo-chrome by default. r=bholley
https://hg.mozilla.org/mozilla-central/rev/1f08bf9a7a50
https://hg.mozilla.org/mozilla-central/rev/7a63a457c8e8
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1440036
Depends on: 1440636
Release Note Request (optional, but appreciated)

[Why is this notable]:
Stylo-chrome should improve multi-core performance of Firefox's browser UI. Stylo-chromem replaces the last big use of Gecko's old style system. We can remove the old code in Nightly 61 (but that won't affect users because the code is already disabled in 60).

[Affects Firefox for Android]: 
No. Stylo-chrome only affects Firefox desktop.

[Suggested wording]:
"Now using the Quantum Style engine for Firefox's browser UI."

[Links (documentation, blog post, etc)]:
There are some blog posts we could link to, but they are about Stylo in general, not Stylo-chrome specifically. I don't think we need more than just a release note about Stylo-chrome.
relnote-firefox: --- → ?
Depends on: 1486166
No longer depends on: 1486166
Regressions: 1563484
You need to log in before you can comment on or make changes to this bug.