stylo: Enable stylo-chrome by default

RESOLVED FIXED in Firefox 60

Status

()

P3
normal
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Depends on: 2 bugs, {feature})

Trunk
mozilla60
feature
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)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Assignee: nobody → xidorn+moz
(Assignee)

Updated

a year ago
Depends on: 1417207
(Assignee)

Updated

a year ago
Depends on: 1417281
(Assignee)

Updated

a year ago
Depends on: 1417548
(Assignee)

Updated

a year ago
Depends on: 1417563
No longer depends on: 1417563
Depends on: 1417563
(Assignee)

Updated

a year ago
Depends on: 1417600
(Assignee)

Comment 1

a year ago
It seems with various patches, the only remaining one now is just bug 1417563: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bed10f14367791b7373c4e3982e692ecf283c35a
(Assignee)

Comment 2

a year ago
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
(Assignee)

Comment 3

a year ago
Hmmm... not sure what's happening to browser/components/contextualidentity/test/browser/browser_aboutURLs.js :/
(Assignee)

Updated

a year ago
Depends on: 1419903
(Assignee)

Updated

a year ago
Depends on: 1419943
(Assignee)

Updated

a year ago
Depends on: 1419554
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
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.
(Assignee)

Comment 10

a year ago
(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.
(Assignee)

Updated

a year ago
Depends on: 1420423
Flags: needinfo?(emilio)
(Assignee)

Updated

a year ago
Depends on: 1421374
(Assignee)

Updated

9 months ago
Depends on: 1436606
(Assignee)

Updated

9 months ago
Depends on: 1437774
(Assignee)

Updated

9 months ago
Depends on: 1437790
(Assignee)

Updated

9 months ago
Depends on: 1437796
(Assignee)

Updated

9 months ago
Depends on: 1438078
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8931481 - Attachment is obsolete: true
Attachment #8931481 - Flags: review?(cam)
(Assignee)

Updated

9 months ago
Attachment #8931482 - Attachment is obsolete: true
Attachment #8931482 - Flags: review?(cam)

Comment 16

9 months ago
mozreview-review
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 17

9 months ago
mozreview-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+

Comment 18

9 months ago
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

Comment 19

9 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd4694fdd87b
followup - Update reftest manifest for stylo-chrome.

Comment 21

9 months ago
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
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Flags: needinfo?(xidorn+moz)

Comment 23

9 months ago
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
No longer depends on: 1438775
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)
(Assignee)

Comment 26

9 months ago
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 :/
(Assignee)

Comment 27

9 months ago
Kato-san, do you have any suggestion about how can we fix that crash?
Flags: needinfo?(xidorn+moz) → needinfo?(m_kato)
(Assignee)

Comment 28

9 months ago
Not intended to remove my own ni?...
Flags: needinfo?(xidorn+moz)
Comment hidden (obsolete)
(Assignee)

Comment 30

9 months ago
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.
(Assignee)

Comment 31

9 months ago
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...
(Assignee)

Comment 32

9 months ago
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
(Assignee)

Comment 33

9 months ago
Created attachment 8951812 [details]
relevant log from Android logcat
(Assignee)

Comment 34

9 months ago
It seems this specific case indeed happened before as shown in bug 1430500. Maybe stylo-chrome makes it happen more reliably?
Depends on: 1430500
(Assignee)

Comment 36

9 months ago
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().
(Assignee)

Comment 37

9 months ago
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

Comment 38

9 months ago
(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?
(Assignee)

Comment 39

9 months ago
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
(Assignee)

Comment 40

9 months ago
(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.
(Assignee)

Comment 41

9 months ago
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.
(Assignee)

Updated

9 months ago
Depends on: 1439406
(Assignee)

Comment 42

9 months ago
Anyway, it seems bug 1439406 should fix that issue.
Flags: needinfo?(xidorn+moz)

Comment 43

9 months ago
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

Comment 44

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f08bf9a7a50
https://hg.mozilla.org/mozilla-central/rev/7a63a457c8e8
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
status-firefox58: --- → wontfix
status-firefox59: affected → wontfix
status-firefox-esr52: --- → unaffected

Updated

9 months ago
Depends on: 1440036

Updated

9 months ago
Depends on: 1440636
Depends on: 1442867
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: --- → ?
relnote-firefox: ? → 60+
Keywords: feature

Updated

3 months ago
Depends on: 1486166
No longer depends on: 1486166
You need to log in before you can comment on or make changes to this bug.