Closed Bug 1471066 Opened 6 years ago Closed 6 years ago

Prevent more modules from being loaded early during a content process startup

Categories

(Core :: DOM: Content Processes, enhancement)

Product:
Core
Component:
DOM: Content Processes
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1471066 - Convert browser_startup_content.js to a whitelist.
59 bytes, text/x-review-board-request
florian
: review+
Details 
As part of the effort from bug 1436250, a significant number of modules that were loaded during early startup of the content process have been made to load later or on demand.

We should prevent these modules from being accidentally loaded earlier again, and also any new modules from being added in the startup path without a valid reason.

There's an existing test called browser_startup_content.js for this, but that test contains a blacklist which hasn't been updated in a while, so its effectiveness has been limited on only preventing the listed things there from being added.

We should convert that test to be a whitelist, and strive to remove more entries from the list.
Comment on attachment 8987729 [details]
Bug 1471066 - Convert browser_startup_content.js to a whitelist.

https://reviewboard.mozilla.org/r/252994/#review259580

Looks good in general. It would be nice to separate in the whitelist the items that are expected to remain here vs the items that should eventually be removed (and it would be even better to include a bug number for each item that should eventually be removed).

::: browser/base/content/test/performance/browser_startup_content.js:93
(Diff revision 1)
> +
> +    // Shield
> +    "resource://normandy-content/AboutPages.jsm",
>    ]),
> +
>    services: new Set([

Can you explain what this does? I have a hard time believing that no xpcom service gets loaded in a new content process ;-).

::: browser/base/content/test/performance/browser_startup_content.js:164
(Diff revision 1)
> +      ok(false, `${scriptType} is whitelisted for content process startup but wasn't used: ${script}`);
> +    }
> +
> +    if (kDumpAllStacks) {
> +      info(`Stacks for all loaded ${scriptType}:`);
> +      for (let file of Object.keys(loadedInfo[scriptType])) {

Is "for (let file of Object.keys(obj))" better than "for (let file in obj)" ?

::: browser/base/content/test/performance/browser_startup_content.js:165
(Diff revision 1)
> +    }
> +
> +    if (kDumpAllStacks) {
> +      info(`Stacks for all loaded ${scriptType}:`);
> +      for (let file of Object.keys(loadedInfo[scriptType])) {
> +        info(`${file}\n------------------------------------\n` + loadedInfo[scriptType][file]);

nit: maybe add an empty line in the output to separate the scripts?
Attachment #8987729 - Flags: review?(florian)
Comment on attachment 8987729 [details]
Bug 1471066 - Convert browser_startup_content.js to a whitelist.

https://reviewboard.mozilla.org/r/252994/#review259580

> Can you explain what this does? I have a hard time believing that no xpcom service gets loaded in a new content process ;-).

Ah, I just assumed the previous code was working, but it wasn't.. There was an existing for..in that should have been for..of.

The list of loaded contract IDs is long.. I added the entire list but I now wonder if this part should be removed from the test (don't know how frequent it will happen that some C++ changes this list). What do you think?

> Is "for (let file of Object.keys(obj))" better than "for (let file in obj)" ?

Not that I know of.. It was just from some previous changes I had made in the patch. Since this was pre-existing code I converted it to the previous style

> nit: maybe add an empty line in the output to separate the scripts?

done
Comment on attachment 8987729 [details]
Bug 1471066 - Convert browser_startup_content.js to a whitelist.

https://reviewboard.mozilla.org/r/252994/#review259646
(In reply to :Felipe Gomes (needinfo me!) from comment #5)
> Comment on attachment 8987729 [details]
> Bug 1471066 - Convert browser_startup_content.js to a whitelist.
> 
> https://reviewboard.mozilla.org/r/252994/#review259646

Hm this was some mozreview weirdness, I didn't mean to clean the review flag. You can take a look at the new patch.

But I just realized that since this list might vary by platform, so I'll send it to tryserver now to see what happens and whether I'll need to add code to dynamically add to the lists based on platform.


(In reply to Florian Quèze [:florian] from comment #2)
> Looks good in general. It would be nice to separate in the whitelist the
> items that are expected to remain here vs the items that should eventually
> be removed (and it would be even better to include a bug number for each
> item that should eventually be removed).

I think the separation in areas is better, and we can have the ones expected to be removed marked by comments with bug numbers.

I added the bug numbers for the ones that I are already tracked (from bug 1436250), but I don't intend to do more analysis on this bug on which ones should eventually be removed.
I'm afraid this list of services is too long to be useful as a whitelist. Should we keep a blacklist for services?
Taking a stab at a better component here (thanks mconley).
Component: General → DOM: Content Processes
(In reply to Florian Quèze [:florian] from comment #8)
> I'm afraid this list of services is too long to be useful as a whitelist.
> Should we keep a blacklist for services?

I converted the services list to be a blacklist, and looked into what more we could add, but there was only one more.

I checked that OS-specific entries won't be needed for this test. (it would be if the services section was kept as a whitelist)
Comment on attachment 8987729 [details]
Bug 1471066 - Convert browser_startup_content.js to a whitelist.

https://reviewboard.mozilla.org/r/252994/#review259986

Looks good, thanks!
Attachment #8987729 - Flags: review?(florian) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2cc73c7d2c75
Convert browser_startup_content.js to a whitelist. r=florian
Blocks: 1471674
https://hg.mozilla.org/mozilla-central/rev/2cc73c7d2c75
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
\o/

Should we emit an error when whitelisted modules aren't loaded, so that we can make sure we remove entries when we get them off of the startup path? And I guess maybe add a flag for modules that are allowed to be loaded but aren't expected to be.
(In reply to Kris Maglione [:kmag] from comment #16)
> \o/
> 
> Should we emit an error when whitelisted modules aren't loaded, so that we
> can make sure we remove entries when we get them off of the startup path?

This is already the case!

> And I guess maybe add a flag for modules that are allowed to be loaded but
> aren't expected to be.

What would be an example of this?

We'll need to do something with the same end effect anyways for modules that are only loaded on certain platforms. All tier 1 stuff surprisingly worked without this, but there are two modules on a tier 2 platform that are being loaded and is causing this test to perma-fail there: bug 1471674
(In reply to :Felipe Gomes (needinfo me!) from comment #17)
> All tier 1 stuff surprisingly worked
> without this, but there are two modules on a tier 2 platform that are being
> loaded and is causing this test to perma-fail there: bug 1471674

I don't think 'Linux x64 pgo' is tier 2, it seems you just have an intermittent failure that happens mostly (only?) in test-verify jobs.
(In reply to :Felipe Gomes (needinfo me!) from comment #17)
> (In reply to Kris Maglione [:kmag] from comment #16)
> > Should we emit an error when whitelisted modules aren't loaded, so that we
> > can make sure we remove entries when we get them off of the startup path?
> 
> This is already the case!

Ah, good. From a quick scan of the patch, it didn't seem to be.

> > And I guess maybe add a flag for modules that are allowed to be loaded but
> > aren't expected to be.
> 
> What would be an example of this?

No idea :) It just wouldn't surprise me if it wound up happening.
Depends on: 1474131
Depends on: 1474140
Depends on: 1474143
Blocks: 1479235
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: