1389443 - nsHandlerService-json.js does main thread IO during startup to load its json file

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P2)

Description

[Florian Quèze [:florian]]

See this startup profile: https://perfht.ml/2uuOkgN

This is happening right after first paint, triggered by PdfJs.init(true); at http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/browser/components/nsBrowserGlue.js#889

I think before running this line we should initialize the handler service asynchronously, using JSONFile.jsm's async 'load' method instead of the synchronous 'ensureDataReady' method.

Comment 1

[:Paolo Amadini]

Looks like bug 1362401? (Although the proposed solution may be different.)

Comment 2

[Florian Quèze [:florian]]

If we just fix bug 1362401, the first thing accessing the handler service will still trigger main thread IO. But yeah, this is a newer version of the same problem.

Comment 3

[Alex Thayer [:alexical] (she/her)]

I have some spare cycled to take this unless anyone else was planning on it.

Comment 4

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1389443 - Load handlers.json asynchronously

Asynchronously load handlers.json for nsHandlerService-json.js in
order to avoid blocking IO early on.

Review commit: https://reviewboard.mozilla.org/r/168876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168876/

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8897600 [details]
Bug 1389443 - Load handlers.json asynchronously

https://reviewboard.mozilla.org/r/168876/#review174424

My suggestion in comment 0 was to delay the PdfJs.init call until we are done loading the file asynchronously. Are you concerned that delaying the PdfJs.init call may cause issues (eg. when session restoring a tab containing a pdf file) ? If so, here is another suggestion:

1. add an asyncInit method to the handler service.
2. call asyncInit from an idle callback in nsBrowserGlue.
3. at the end of the handler service's initialization (both the async and the sync initialization methods), send an observer notification (or resolve a promise), from which we can initialize pdf.js.

This should let us initialize everything asynchronously and without jank if we don't need this data near startup. And if we do need to have pdf.js initialized immediately, then it'll be done synchronously before we attempt to load a first pdf file.

::: browser/components/nsBrowserGlue.js:599
(Diff revision 1)
>      // apply distribution customizations
>      // prefs are applied in _onAppDefaults()
>      this._distributionCustomizer.applyCustomizations();
>  
> +    let handlerService = Cc["@mozilla.org/uriloader/handler-service;1"].
> +                         getService(Ci.nsIHandlerService);

Let's not instantiate this service before first paint.

::: uriloader/exthandler/nsHandlerService-json.js:247
(Diff revision 1)
> +    if (!this.__store) {
> +      this.__store = new JSONFile({
> +        path: OS.Path.join(OS.Constants.Path.profileDir, "handlers.json"),
> +        dataPostProcessor: this._dataPostProcessor.bind(this),
> +      });
> +      this.__store.load();

This offers no guarantee that it'll be finished before the existing PdfJs.init(true); call.

Comment 6

[:Paolo Amadini]

You can take a look at the Logins Manager code because we also allow both asynchronous and synchronous initialization. The code there is already designed to avoid race conditions.

Comment 7

[Alex Thayer [:alexical] (she/her)]

The problem I run into when loading PdfJs async is that we also have to move the subsequent PdfJs.enabled call[1] to be async, or else we end up synchronously initializing the nsHandlerService-json.js anyway. However, if we move the loadProcessScript call to be async, we aren't registered in time to correctly display PDFs when session restoring a PDF file. Do you have any suggestions for how to deal with this case? I'll keep exploring options, but advice is welcome.

[1]: http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/browser/components/nsBrowserGlue.js#873

Comment 8

[Florian Quèze [:florian]]

(In reply to Doug Thayer [:dthayer] from comment #7)
> if
> we move the loadProcessScript call to be async, we aren't registered in time
> to correctly display PDFs when session restoring a PDF file.

Is this case possible to detect? If we can detect it, I think it's fine to initialize the handler service synchronously in that case.

Comment 9

[Alex Thayer [:alexical] (she/her)]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Is this case possible to detect? If we can detect it, I think it's fine to
> initialize the handler service synchronously in that case.

The best I can come up with is to guess from the URL in SessionRestore or when loading a custom home page. The registration of the PdfJsStreamConverter is what we need to have ready before we would want to start streaming data through it, and I'm not aware of how to stop that train in order to synchronously register ourselves once we know for sure what the MIME type is, but I believe doing so would require synchronous IPC at minimum.

Other options might be recording whether each document in our session is a pdf or not so that we can have that information easily on restore, or reloading the page if we detect that we need to reprocess the stream, though that seems a bit excessive.

Comment 10

[Florian Quèze [:florian]]

What happens if we attempt to load a pdf file and PdfJsStreamConverter isn't registered yet? If this goes through the handler service to find an external application to handle it, this may give us an opportunity to synchronously initialize.

I'm also wondering how likely we are to session restore a pdf file, as I think most tabs are lazy loaded during session restore.

Comment 11

[Florian Quèze [:florian]]

Paolo, do you have suggestions about what could be a good way forward here?

Comment 12

[:Paolo Amadini]

The right solution seems way too complicated, it would involve always registering the PdfStreamConverter and having it forward the data to any previously registered converter once it determined from the asynchronous initialization of nsIHandlerService that the internal PDF viewer is not enabled. This would have to suspend the channel while the determination is done.

There is bug 1362564 on file to make the handler determination process asynchronous, but it will not be trivial to implement.

As a workaround, I suggest creating a surrogate preference that tells whether the internal PDF viewer is enabled, and use that to determine if we should register the PdfStreamConverter on startup. Then we can listen to changes to the other preferences and update it accordingly. There are already observers in PDF.js for when the plugin states change, we could add an observer mechanism for changes to the nsIHandlerService store and listen to those as well.

The default value of the preference should match the initial state of PDF.js for new profiles, and in case we've just upgraded from a previous version where the preference doesn't exist, we should still run the determination process synchronously on startup and set the preference afterwards.

We can also run an idle task, or better add an observer for when the nsIHandlerService store is first loaded, to ensure we keep the preference in sync, just in case something like a crash left it in the wrong state.

Please add regression tests if you add any observer notifications to the nsIHandlerService back-end.

Comment 13

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8897600 [details]
Bug 1389443 - Load handlers.json asynchronously

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168876/diff/1-2/

Comment 14

[:Paolo Amadini]

Comment on attachment 8897600 [details]
Bug 1389443 - Load handlers.json asynchronously

https://reviewboard.mozilla.org/r/168876/#review175056

Remember to add regression tests for the observer notification and for the interaction between init and asyncInit. There is something similar in "test_module_LoginStore.js", but you may want to race the two methods here.

::: uriloader/exthandler/nsHandlerService-json.js:249
(Diff revision 2)
> +  async asyncInit() {
> +    if (!this.__store) {
> +      this.__store = new JSONFile({
> +        path: OS.Path.join(OS.Constants.Path.profileDir, "handlers.json"),
> +        dataPostProcessor: this._dataPostProcessor.bind(this),
> +      });
> +      await this.__store.load();
> +      this._ensureStoreInitialized();
> +    }
> +  },

Since asyncInit returns void, this should be:

asyncInit() {
  (async () => {
    // ...
  })().catch(Cu.reportError);
}

This avoids leaving unhandled rejections. Alternatively you can declare the function as returning a jsval so consumers will call "catch", but then you have to store the Promise somewhere to handle re-entrancy.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8897600 [details]
Bug 1389443 - Load handlers.json asynchronously

https://reviewboard.mozilla.org/r/168876/#review175248

Looks good to me, thanks! I would suggest requesting the next review from Paolo, as he requested tests in comment 14.

Touching the browser/extensions/pdfjs folder usually requires at least opening a new issue in https://github.com/mozilla/pdf.js/ so that the changes can be ported upstream. This is a non-trivial change, so we probably need to get a review from a pdfjs peer.

::: browser/components/nsBrowserGlue.js:871
(Diff revision 2)
> -    // leaving initialization to the extension.
> -    // parent only: configure default prefs, set up pref observers, register
> -    // pdf content handler, and initializes parent side message manager
> -    // shim for privileged api access.
> -    PdfJs.init(true);
>      // child only: similar to the call above for parent - register content

This comment needs adjusting as "the call above for parent" doesn't make sense anymore here.

::: uriloader/exthandler/nsHandlerService-json.js:249
(Diff revision 2)
> +  async asyncInit() {
> +    if (!this.__store) {
> +      this.__store = new JSONFile({
> +        path: OS.Path.join(OS.Constants.Path.profileDir, "handlers.json"),
> +        dataPostProcessor: this._dataPostProcessor.bind(this),
> +      });
> +      await this.__store.load();
> +      this._ensureStoreInitialized();
> +    }
> +  },

Or just make this a normal (non async) function, and do:
this.__store.load().then(() => { this._ensureStoreInitialized(); }, Cu.reportError);

::: uriloader/exthandler/nsHandlerService.js:306
(Diff revision 2)
>  
>  
>    //**************************************************************************//
>    // nsIHandlerService
> +  asyncInit: function HS_asyncInit() {
> +    // noop

I'm not sure in which case this old nsHandlerServices.js component is still used, but I wonder if we need to make it somehow send the handlersvc-store-initialized notification.

Edit: Paolo just told me it's for migration, so if it's only going to be called from nsHandlerService-json.js, we don't need it to send any notification.

Comment 16

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> Or just make this a normal (non async) function, and do:
> this.__store.load().then(() => { this._ensureStoreInitialized(); },
> Cu.reportError);

Good suggestion, but one correction:

.then(() => this._ensureStoreInitialized()).catch(Cu.reportError);

(We should probably have an ESLint rule to discourage the second parameter to "then".)

Comment 17

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8897600 [details]
Bug 1389443 - Load handlers.json asynchronously

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168876/diff/2-3/

Comment 18

[:Paolo Amadini]

Comment on attachment 8897600 [details]
Bug 1389443 - Load handlers.json asynchronously

https://reviewboard.mozilla.org/r/168876/#review176450

r+ for the nsIHandlerService changes. Thanks!

As Florian noted, you likely need a review from a pdfjs peer. He might also want to take another look at the pdfjs changes, I haven't reviewed them in detail.

Comment 19

[Alex Thayer [:alexical] (she/her)]

(In reply to :Paolo Amadini from comment #18)
> As Florian noted, you likely need a review from a pdfjs peer. He might also
> want to take another look at the pdfjs changes, I haven't reviewed them in
> detail.

Created a PR[1] for this. Brendan, would you mind taking a look?


[1]: https://github.com/mozilla/pdf.js/pull/8810

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8897600 [details]
Bug 1389443 - Load handlers.json asynchronously

https://reviewboard.mozilla.org/r/168876/#review176772

Looks good to me, thanks!

Comment 21

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8897600 [details]
Bug 1389443 - Load handlers.json asynchronously

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168876/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22ad20546ae7
Load handlers.json asynchronously r=florian,Paolo

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/22ad20546ae7

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

== Change summary for alert #8972 (as of August 23 2017 21:01 UTC) ==

Improvements:

  8%  sessionrestore_no_auto_restore windows7-32 opt e10s     773.54 -> 709.83
  8%  sessionrestore windows7-32 opt e10s                     684.17 -> 630.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8972