1446423 - Startup race of nsILoadUriDelegate registration

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Eugen Sawin [:esawin]]

With e10s disabled, the nsIUriLoadDelegate may not be registered in time in the GeckoViewNavigationContent.js frame script before a loadUri request is processed.
This results in a missing onLoadRequest call and potentially an unexpected call to onNewSession.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1446423-1.0-Confirm-content-module-registeration.patch

By adding a registration ACK in frame scripts, we can resolve the contentReady promise which would make async handling of events that depend on the content script possible.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8959570 [details] [diff] [review]
0001-Bug-1446423-1.0-Confirm-content-module-registeration.patch

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

I think this is going in the right direction, but I would like to see the more explicit queuing of events while we're waiting on the content script to load.

::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm
@@ +38,5 @@
>          if (aMsg.data.module == this.moduleName) {
>            this.settings = aMsg.data.settings;
>            this.register();
> +          this.messageManager.sendAsyncMessage(
> +            "GeckoView:RegisterContentAck", { module: this.moduleName });

GeckoView:ContentRegistered maybe?

::: mobile/android/modules/geckoview/GeckoViewModule.jsm
@@ +79,5 @@
> +    }
> +    this.isContentLoaded = true;
> +
> +    this.contentReady = new Promise(resolve => {
> +      let self = this;

Use an arrow function below to avoid this

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm
@@ +43,5 @@
>    }
>  
>    // Bundle event handler.
>    onEvent(aEvent, aData, aCallback) {
>      debug("onEvent: aEvent=" + aEvent + ", aData=" + JSON.stringify(aData));

I would move this inside the then() so it's only printed when we actually taking action

@@ +45,5 @@
>    // Bundle event handler.
>    onEvent(aEvent, aData, aCallback) {
>      debug("onEvent: aEvent=" + aEvent + ", aData=" + JSON.stringify(aData));
>  
> +    this.contentReady.then(() => {

I am not sure if the order of arrival will be preserved when this promise gets resolved.

I would prefer if we added registerListener() to GeckoViewModule, and then GVM would take care of dispatching to onEvent() by hooking up its own internal listener only after contentReady() resolved. I think it would just keep a queue that it would later flush instead of relying on the order being right during promise resolution.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8959570 [details] [diff] [review]
0001-Bug-1446423-1.0-Confirm-content-module-registeration.patch

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

I think this is going in the right direction, but I would like to see the more explicit queuing of events while we're waiting on the content script to load.

::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm
@@ +38,5 @@
>          if (aMsg.data.module == this.moduleName) {
>            this.settings = aMsg.data.settings;
>            this.register();
> +          this.messageManager.sendAsyncMessage(
> +            "GeckoView:RegisterContentAck", { module: this.moduleName });

GeckoView:ContentRegistered maybe?

::: mobile/android/modules/geckoview/GeckoViewModule.jsm
@@ +79,5 @@
> +    }
> +    this.isContentLoaded = true;
> +
> +    this.contentReady = new Promise(resolve => {
> +      let self = this;

Use an arrow function below to avoid this

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm
@@ +43,5 @@
>    }
>  
>    // Bundle event handler.
>    onEvent(aEvent, aData, aCallback) {
>      debug("onEvent: aEvent=" + aEvent + ", aData=" + JSON.stringify(aData));

I would move this inside the then() so it's only printed when we actually taking action

@@ +45,5 @@
>    // Bundle event handler.
>    onEvent(aEvent, aData, aCallback) {
>      debug("onEvent: aEvent=" + aEvent + ", aData=" + JSON.stringify(aData));
>  
> +    this.contentReady.then(() => {

I am not sure if the order of arrival will be preserved when this promise gets resolved.

I would prefer if we added registerListener() to GeckoViewModule, and then GVM would take care of dispatching to onEvent() by hooking up its own internal listener only after contentReady() resolved. I think it would just keep a queue that it would later flush instead of relying on the order being right during promise resolution.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1446423-1.1-Queue-bundle-events-during-content-m.patch

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> I think this is going in the right direction, but I would like to see the
> more explicit queuing of events while we're waiting on the content script to
> load.

The EventProxy forwards the events directly to its module when queuing is disabled or queues them until we explicitly call dispatchQueuedEvents when queuing is enabled. This should preserve the order of the events on the Gecko thread.

Note that, with e10s enabled, we could still load the pages in reverse order. We should address that issue in bug 1441059.

> Use an arrow function below to avoid this

I need a named function to unregister the listener in the callback.

Comment 5

[Eugen Sawin [:esawin]]

Auto-unregistration is a bad idea after all, we can't know if registerListener was called in register or init.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8960339 [details] [diff] [review]
0001-Bug-1446423-1.1-Queue-bundle-events-during-content-m.patch

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

r+ because I want to get the race solved, but now think handling in EventDispatcher might be nicer

::: mobile/android/modules/geckoview/GeckoViewModule.jsm
@@ +118,5 @@
>      return this.browser.messageManager;
>    }
>  }
> +
> +class EventProxy {

Hmm, this all seems more complex than necessary.

I wonder if it would be useful to tell the EventDispatcher itself to pause while we wait for the content script to start. Then we can just unpause() it and all the queueing, etc, would be in the EventDispatcher which conceivably already knows how to do this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73439b2acd64
[1.1] Queue bundle events during content module registeration. r=snorp

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/73439b2acd64

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3dfc249b9a
[2.0] Fix typo. r=me

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ec3dfc249b9a