864494 - System principal merging is only happening on the main process

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 798491 helped greatly, but it looks like it's only being effective for the main process:

> 13,642,508 B (100.0%) -- explicit
> ├───4,403,684 B (32.28%) -- js-non-window
> │   ├──3,865,124 B (28.33%) -- compartments
> │   │  ├──3,358,912 B (24.62%) -- non-window-global
> │   │  │  ├────277,688 B (02.04%) ++ compartment([System Principal])
> │   │  │  ├────262,212 B (01.92%) ++ compartment([System Principal], resource://gre/modules/DOMRequestHelper.jsm)
> │   │  │  ├────199,328 B (01.46%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/Webapps.js)
> │   │  │  ├────187,400 B (01.37%) ++ compartment([System Principal], resource://gre/modules/XPCOMUtils.jsm)
> │   │  │  ├────182,632 B (01.34%) ++ compartment([System Principal], resource://gre/modules/CSPUtils.jsm)
> │   │  │  ├────134,456 B (00.99%) ++ compartment([System Principal], resource://gre/modules/ObjectWrapper.jsm)
> │   │  │  ├────124,216 B (00.91%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/SettingsManager.js)
> │   │  │  ├────114,360 B (00.84%) ++ compartment([System Principal], resource://gre/modules/UserAgentOverrides.jsm)
> │   │  │  ├────110,008 B (00.81%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/contentSecurityPolicy.js)
> │   │  │  ├────107,012 B (00.78%) ++ compartment([System Principal], resource://gre/modules/AppsServiceChild.jsm)
> │   │  │  ├────105,264 B (00.77%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/PushService.js)
> │   │  │  ├────104,688 B (00.77%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/DirectoryProvider.js)
> │   │  │  ├─────98,664 B (00.72%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/nsHandlerService.js)
> │   │  │  ├─────92,656 B (00.68%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/nsPrompter.js)
> │   │  │  ├─────89,624 B (00.66%) ++ compartment([System Principal], resource://gre/modules/BrowserElementPromptService.jsm)
> │   │  │  ├─────83,176 B (00.61%) ++ compartment([System Principal], resource://gre/modules/AppsUtils.jsm)
> │   │  │  ├─────82,712 B (00.61%) ++ compartment([System Principal], resource://gre/modules/services-common/preferences.js)
> │   │  │  ├─────81,536 B (00.60%) ++ compartment([System Principal], resource://gre/modules/BrowserElementParent.jsm)
> │   │  │  ├─────77,520 B (00.57%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/AppProtocolHandler.js)
> │   │  │  ├─────70,168 B (00.51%) ++ compartment([System Principal], resource://gre/modules/Geometry.jsm)
> │   │  │  ├─────69,648 B (00.51%) ++ compartment([System Principal], resource://gre/modules/SettingsDB.jsm)
> │   │  │  ├─────66,392 B (00.49%) ++ compartment([System Principal], resource://gre/modules/NetUtil.jsm)
> │   │  │  ├─────65,768 B (00.48%) ++ compartment([System Principal], resource://gre/modules/Services.jsm)
> │   │  │  ├─────65,112 B (00.48%) ++ compartment([System Principal], resource://gre/modules/commonjs/promise/core.js)
> │   │  │  ├─────64,880 B (00.48%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/BrowserElementParent.js)
> │   │  │  ├─────64,864 B (00.48%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/AppsService.js)
> │   │  │  ├─────64,424 B (00.47%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/ProcessGlobal.js)
> │   │  │  ├─────63,656 B (00.47%) ++ compartment([System Principal], jar:file:///system/b2g/omni.ja!/components/nsIDService.js)
> │   │  │  ├─────60,472 B (00.44%) ++ compartment([System Principal], resource://gre/modules/IndexedDBHelper.jsm)
> │   │  │  ├─────60,408 B (00.44%) ++ compartment([System Principal], resource://gre/modules/FileUtils.jsm)
> │   │  │  ├─────58,360 B (00.43%) ++ compartment([System Principal], resource://gre/modules/SettingsQueue.jsm)
> │   │  │  ├─────43,560 B (00.32%) ++ compartment(moz-nullprincipal:{0cf61a20-96af-456a-ba29-ca139251bdd6})
> │   │  │  └─────26,048 B (00.19%) ++ compartment(null-principal)

jlebar said "It could be that we're reading this particular pref before prefs are loaded.  gsvelto saw that happening in a bunch of places."

Comment 1

[Gabriele Svelto [:gsvelto]]

Yes, I saw this happening for a number of prefs and I've made a meta-bug for it, check bug 814517. I'll take this and investigate what's going on here.

Comment 2

[Gabriele Svelto [:gsvelto]]

I can confirm that we're reading "jsloader.reuseGlobal" too early both on b2g18 and m-c, see here:

https://mxr.mozilla.org/mozilla-b2g18/source/js/xpconnect/loader/mozJSComponentLoader.cpp#453

https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#454

I'm not sure if we want to fix it though for two reasons:

- I don't think this can be flipped on or off at runtime without causing issues

- It's still being read as 'true' by the main process (as it's declared that way in b2g/app/b2g.js) and content processes inherit the prefs so they'll also get 'true' for it (I've checked this just in case)

In b2g18 we have two more places reading the pref but both aren't doing anything with the returned value which is... puzzling:

https://mxr.mozilla.org/mozilla-b2g18/source/toolkit/components/ctypes/ctypes.cpp#123
https://mxr.mozilla.org/mozilla-b2g18/source/toolkit/components/perf/PerfMeasurement.cpp#89

We hit the first one in the b2g main process once and we never hit it again either there or in the content processes.

Comment 3

[Gabriele Svelto [:gsvelto]]

I just grabbed another about:memory report from my Unagi with today's build and I can't see the compartments showed reported above. All content processes I've launched seem to have only 3 compartments under explicit -- js-non-window -- compartments -- non-window-global. They pretty much all look like this (this is the Template app):

5.25 MB (100.0%) -- explicit
├──1.65 MB (31.48%) -- js-non-window
│  ├──1.37 MB (26.17%) -- compartments
│  │  ├──1.01 MB (19.29%) -- non-window-global
│  │  │  ├──0.95 MB (18.02%) ++ compartment([System Principal])
│  │  │  └──0.07 MB (01.27%) -- (2 tiny)
│  │  │     ├──0.04 MB (00.79%) ++ compartment(moz-nullprincipal:{bd61cb82-b407-4357-a1a3-88a3780bd907})
│  │  │     └──0.02 MB (00.47%) ++ compartment(null-principal)

Comment 4

[Justin Lebar (not reading bugmail)]

> I'm not sure if we want to fix it though for two reasons:

I don't understand what you mean by this.  What is it that we don't want to fix?  Surely we want to fix this so that compartment merging happens in the child process?

Comment 5

[Gabriele Svelto [:gsvelto]]

(In reply to Justin Lebar [:jlebar] from comment #4)
> I don't understand what you mean by this.  What is it that we don't want to
> fix?  Surely we want to fix this so that compartment merging happens in the
> child process?

I meant we don't want to fix reading the pref too early as that's probably impossible to do reliably. This bug should be fixed however but I couldn't reproduce the issue on my handset using a v1-train build (or maybe I'm looking at the wrong stuff).

Comment 6

[Justin Lebar (not reading bugmail)]

Kyle, this is some of the lowest-hanging memshrink fruit we have on b2g right now.  It doesn't seem hard to hardcode this pref on in child processes, but it's not clear to me whether this would actually be safe, and I don't know how to tell.

Do you want to take this bug, or at least tell us how to judge whether enabling this in the child processes would be safe?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

As long as app code is running with a non-system principal (which I'm 99.9% sure is the case) I expect this would be pretty safe.  Hardcoding is ugly but it's easy, so I don't see any reason not to try it.

Comment 8

[Justin Lebar (not reading bugmail)]

> As long as app code is running with a non-system principal (which I'm 99.9% sure is the case) I 
> expect this would be pretty safe.

App code is content, so I sure hope it's not running with the system principal.  Certainly none of the compartments in comment 0 is app code.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

FWIW I couldn't reproduce this on b2g desktop.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Hardcode it

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 744691 [details] [diff] [review]
Hardcode it

Maybe you could add a comment?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm inclined to think we should just land this on b2g18, and figure out wtf is wrong with prefs.  Anyone disagree?

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> I'm inclined to think we should just land this on b2g18, and figure out wtf
> is wrong with prefs.  Anyone disagree?

Are you willing to take that new bug?

Anyway I have no problem taking this on b2g18 or trunk.

My fear is that we're keeping "risky" changes off b2g18, and then when we switch to m-c, we're going to have tons of debt from all the regressions we introduced and didn't notice.  In this case, I guess the concern would be that we'd land this on b2g18 and then never fix it properly on trunk.

Comment 14

[Justin Lebar (not reading bugmail)]

Landing ping?

Comment 15

[Justin Lebar (not reading bugmail)]

I wanted to measure the improvement here, but I can't seem to reproduce this bug locally.  But that means that this change should be quite safe.  :)

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/7d7d3fb2bdf5

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 744691 [details] [diff] [review]
Hardcode it

[Triage Comment]
I'd like us to take this on b2g18.  The risk of not taking it and having the current behavior (which seems to be undefined) is IMO higher than the risk of taking it.

I'd also like us to take this on 1.0.1 for the same reason, but I'm not going to tef+ it myself.

This patch is obviously trivial to back out if we see problems.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/66334a3bddcb

Comment 19

[Daniel Coloma:dcoloma]

Blocking based on comment 17

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a59ba16d4af8

Comment 21

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't think we were ever able to reliably reproduce this.