Closed Bug 1446423 Opened 2 years ago Closed 2 years ago

Startup race of nsILoadUriDelegate registration

Categories

(GeckoView :: General, defect, P1)

51 Branch
All
Android
defect

Tracking

(firefox-esr52 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Summary: Startup race of nsIUriLoadDelegate registration → Startup race of nsILoadUriDelegate registration
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.
Attachment #8959570 - Flags: review?(snorp)
Blocks: 1440592
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 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.
Attachment #8959570 - Flags: review?(snorp) → review-
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Attachment #8959570 - Attachment is obsolete: true
Attachment #8960339 - Flags: review?(snorp)
Auto-unregistration is a bad idea after all, we can't know if registerListener was called in register or init.
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.
Attachment #8960339 - Flags: review?(snorp) → review+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73439b2acd64
[1.1] Queue bundle events during content module registeration. r=snorp
https://hg.mozilla.org/mozilla-central/rev/73439b2acd64
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/ec3dfc249b9a
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.