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)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
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 5•6 years ago
|
||
mozreview-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 6•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
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
Comment 13•6 years ago
|
||
bugherder |
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
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•5 years ago
|
Product: Firefox for Android → GeckoView
Updated•5 years ago
|
Target Milestone: Firefox 61 → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•