Closed Bug 1454038 Opened 6 years ago Closed 6 years ago

GeckoView:MessagingReply cannot be sent after browser is destroyed

Categories

(GeckoView :: General, enhancement, P2)

All
Android
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(3 files)

We can potentially send GeckoView:MessagingReply messages after the browser has been destroyed, and it fails because we no longer have a message manager.
Comment on attachment 8968279 [details]
Bug 1454038 - 1. Share EventDispatcher among frame scripts;

https://reviewboard.mozilla.org/r/236962/#review242974

::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm:28
(Diff revision 1)
>  
>    static create(aGlobal, aModuleName) {
>      return new this(aModuleName || this._moduleName, aGlobal);
>    }
>  
> -  constructor(aModuleName, aMessageManager) {
> +  constructor(aModuleName, aGlobal) {

aGlobalMessageManager maybe?
Attachment #8968279 - Flags: review?(esawin) → review+
Comment on attachment 8968280 [details]
Bug 1454038 - 2. Finalize event callbacks on content window unload;

https://reviewboard.mozilla.org/r/236964/#review242976

::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm:36
(Diff revision 1)
>  
>      if (!aGlobal._gvEventDispatcher) {
>        aGlobal._gvEventDispatcher = EventDispatcher.forMessageManager(aGlobal);
> +      aGlobal.addEventListener("unload", event => {
> +        if (event.target === this.messageManager) {
> +          aGlobal._gvEventDispatcher.finalize();

Do we need to prevent multiple calls to this?

::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm:39
(Diff revision 1)
> +      aGlobal.addEventListener("unload", event => {
> +        if (event.target === this.messageManager) {
> +          aGlobal._gvEventDispatcher.finalize();
> +        }
> +      }, {
> +        mozSystemGroup: true,

What does that do?
Attachment #8968280 - Flags: review?(esawin) → review+
Comment on attachment 8968281 [details]
Bug 1454038 - 3. Reject sendRequestForResult promise on finalize;

https://reviewboard.mozilla.org/r/236966/#review242980
Attachment #8968281 - Flags: review?(esawin) → review+
Comment on attachment 8968279 [details]
Bug 1454038 - 1. Share EventDispatcher among frame scripts;

https://reviewboard.mozilla.org/r/236962/#review242974

> aGlobalMessageManager maybe?

We actually pass the frame script global object into here, not a message manager.
Comment on attachment 8968280 [details]
Bug 1454038 - 2. Finalize event callbacks on content window unload;

https://reviewboard.mozilla.org/r/236964/#review242976

> Do we need to prevent multiple calls to this?

Should only be called once.

> What does that do?

It adds the listener to the system group, so content listeners cannot affect the way we receive events.
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b572a19f4d5b
1. Share EventDispatcher among frame scripts; r=esawin
https://hg.mozilla.org/integration/autoland/rev/e4e4dd6ca0c9
2. Finalize event callbacks on content window unload; r=esawin
https://hg.mozilla.org/integration/autoland/rev/baf6063071b5
3. Reject sendRequestForResult promise on finalize; r=esawin
https://hg.mozilla.org/mozilla-central/rev/b572a19f4d5b
https://hg.mozilla.org/mozilla-central/rev/e4e4dd6ca0c9
https://hg.mozilla.org/mozilla-central/rev/baf6063071b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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.