ContextualIdentityService should not be initialized before first paint

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: florian, Assigned: baku)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [userContextId][domsecurity-backlog])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 months ago
See this profile for what the code currently does while creating the first browser window: https://perfht.ml/2qIfZVz

Things that should be changed:

- the imports in browser.js, nsContextMenu.js and utilityOverlay.js should be lazy

- ContextualIdentityService.load() should be called off an idle dispatch callback from nsBrowserGlue.js' _onWindowsRestored method.

- this._saver = new DeferredTask(... should be done the first time we call saveSoon

- the AsyncShutdown blocker should only be set when there's pending data to write, to avoid needlessly doing work during shutdown when there's nothing to save.
Florin, in terms of priority, should we get this fixed for the fx57 proton/photon release? Currently, most the work is going into the test pilot experiment [1] and the shield study [2].

[1] https://github.com/mozilla/testpilot-containers/
[2] https://github.com/mozilla/testpilot-containers/labels/shield
Blocks: 1191418
Whiteboard: [userContextId][domsecurity-backlog]
(Reporter)

Comment 2

3 months ago
(In reply to Kamil Jozwiak [:kjozwiak] from comment #1)
> Florin, in terms of priority, should we get this fixed for the fx57
> proton/photon release?

Yes. Unless I missed something, it's going to be easy (I explained almost everything that should be done in comment 0).
baku, mind taking a look?
Flags: needinfo?(amarchesini)
Priority: -- → P1
Created attachment 8874085 [details] [diff] [review]
contextual.patch
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8874085 - Flags: review?(florian)
Status: NEW → ASSIGNED
(Reporter)

Comment 5

2 months ago
Comment on attachment 8874085 [details] [diff] [review]
contextual.patch

Review of attachment 8874085 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking at this!

::: browser/components/nsBrowserGlue.js
@@ +1181,5 @@
>        }
>      }
>  
> +    // Let's load the contextual identities.
> +    ContextualIdentityService.load();

In comment 0 I suggested doing this off an idle dispatch, ie.
Services.tm.mainThread.idleDispatch(() => {
  ContextualIdentityService.load();
});

Did you just miss this suggestion, or is there a reason why it's not a good idea?

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +155,5 @@
> +      AsyncShutdown.profileBeforeChange.addBlocker(
> +        "ContextualIdentityService: writing data",
> +        () => {
> +          if (this._saverCount) {
> +            this._saver.finalize();

you forgot a 'return' keyword here.

@@ +160,5 @@
> +          }
> +        });
> +    }
> +
> +    ++this._saverCount;

I don't understand what this _saverCount int is for.

If you are concerned about something changing the data while we are already in the process of saving to disk (ie. after the timer has fired), you can call .disarm() and then .arm() again on the deferred task.

@@ +175,5 @@
>     };
>  
>     let bytes = gTextEncoder.encode(JSON.stringify(object));
>     return OS.File.writeAtomic(this._path, bytes,
>                                { tmpPath: this._path + ".tmp" });

Can we do:

let promise = OS.File.writeAtomic(...)
return promise.then(() => {
  this._saver = null;
  AsyncShutdown.profileBeforeChange.removeBlocker(promise);
});

I'm suggesting this because I would prefer if no ContextualIdentityService.jsm code at all ran during shutdown, unless we have something that needs to be saved to disk.
Attachment #8874085 - Flags: review?(florian) → feedback+
Created attachment 8876777 [details] [diff] [review]
contextual.patch
Attachment #8874085 - Attachment is obsolete: true
Attachment #8876777 - Flags: review?(florian)
(Reporter)

Comment 7

2 months ago
Comment on attachment 8876777 [details] [diff] [review]
contextual.patch

Review of attachment 8876777 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I see I made a mistake in comment 5 that was likely confusing :-(.

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +152,5 @@
> +    if (!this._saver) {
> +      this._saver = new DeferredTask(() => this.save(), SAVE_DELAY_MS);
> +      AsyncShutdown.profileBeforeChange.addBlocker(
> +        "ContextualIdentityService: writing data",
> +        () => this._saver.finalize());

We'll need to save a reference to this callback, to be able to remove the shutdown blocker later.

@@ +171,5 @@
>     let bytes = gTextEncoder.encode(JSON.stringify(object));
> +   let promise = OS.File.writeAtomic(this._path, bytes,
> +                                     { tmpPath: this._path + ".tmp" });
> +
> +   AsyncShutdown.profileBeforeChange.addBlocker(

OS File already handles for you adding a shutdown blocker, see http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/toolkit/components/osfile/modules/osfile_async_front.jsm#1496

@@ +175,5 @@
> +   AsyncShutdown.profileBeforeChange.addBlocker(
> +     "ContextualIdentityService: writing data", promise);
> +
> +   return promise.then(() => {
> +     AsyncShutdown.profileBeforeChange.removeBlocker(promise);

The shutdown blocker I would like us to remove from the save method is the blocker added in the saveSoon method. And I would like us to set this._saver to null too, so that we re-add the blocker the next time a save is needed.
Attachment #8876777 - Flags: review?(florian) → feedback+
Created attachment 8877040 [details] [diff] [review]
contextual.patch
Attachment #8876777 - Attachment is obsolete: true
Attachment #8877040 - Flags: review?(florian)
(Reporter)

Comment 9

2 months ago
Comment on attachment 8877040 [details] [diff] [review]
contextual.patch

Review of attachment 8877040 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good to me now, with one last comment that needs fixing.

Also, I would appreciate if you could include test coverage, which is very easy to do now: you just need to blacklist the "resource://gre/modules/ContextualIdentityService.jsm" module in http://searchfox.org/mozilla-central/source/browser/base/content/test/performance/browser_startup.js

To verify this bug is fixed, you need to black list it at least for the "before first paint" startup phase... but given that you used idleDispatch, you can likely blacklist it for all of startup (ie. in the "before handling user events" phase) if it passes try :-).

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +149,5 @@
>    },
>  
>    saveSoon() {
> +    if (!this._saver) {
> +      this._saverCallback = () => { this._saver.finalize(); }

You need the shutdown blocker to return the promise returned by .finalize().
So this line should either become:
 this._saverCallback = () => this._saver.finalize();
or if you want to be very explicit:
 this._saverCallback = () => { return this._saver.finalize(); }

I would personally go with the first solution, but either is fine.
Attachment #8877040 - Flags: review?(florian) → review+

Comment 10

2 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28ae65a052aa
ContextualIdentityService should not be initialized before first paint, r=florian

Comment 11

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28ae65a052aa
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.