Closed Bug 1426154 Opened 2 years ago Closed 2 years ago

Use targetted message manager to communciate with content browsers

Categories

(Testing :: Marionette, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Marionette currently uses a message manager broadcaster to communicate
with content browsers.  This is hugely wasteful and fragile,
especially because we use the content browser’s outerWindowID as
the marker for which frame script picks up the message because it
changes when the browser suffers a remoteness change.

We should instead use a targetted
message manager.  The window refactoring work in
https://bugzilla.mozilla.org/show_bug.cgi?id=marionette-window-tracking
makes this possible.

One problem the window refactoring does not take care of is the
Marionette:listenersAttached and this.curBrowser.pendingCommands queue.
I believe we still need some version of this, in case the content
browser disappears whilst processing a message we have sent it.
We are still liable to see this despite improving the tracking of
browsers and windows.
Btw the framescript could listen for the `unload` event on the tabchildglobal to send the parent a message about its unload. That should help us to better handle such situations.
We could perhaps use the unload event to tell chrome to delay, or
more likely re-send, the last message.  Provided it fires consistently,
of course.
I tested that locally for a patch on a different bug, and it was working fine. Only sending the data to the parent process wasn't working that well at that time.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIMessageListenerManager#addMessageListener()
It looks like we might be able to employ a targetted message
manager for communicating directly with content frames without the
window tracking refactoring.  It might make sense to reverse the
relationship and try to land this first.
Assignee: nobody → ato
No longer depends on: marionette-window-tracking
Blocks: 1157192
Great to see the progress on this bug Andreas! I hope that this will already fix the multi-process issues which I have seen on bug 1422769.
Blocks: 1422769
Taskcluster had some kind of major cockup yesterday that ruined
the test run.  Triggered a new one.
Comment on attachment 8941180 [details]
Bug 1426154 - Use targetted IPC messages to talk to content frames.

https://reviewboard.mozilla.org/r/211446/#review217508
Attachment #8941180 - Flags: review?(dburns) → review+
Comment on attachment 8941181 [details]
Bug 1426154 - Sort message listeners lexicographically.

https://reviewboard.mozilla.org/r/211448/#review217512
Attachment #8941181 - Flags: review?(dburns) → review+
Comment on attachment 8941182 [details]
Bug 1426154 - Refactor Marionette content frame script registration.

https://reviewboard.mozilla.org/r/211450/#review217514
Attachment #8941182 - Flags: review?(dburns) → review+
Comment on attachment 8941183 [details]
Bug 1426154 - Rename Marionette:deleteSession to Marionette:Deregister.

https://reviewboard.mozilla.org/r/211452/#review217518
Attachment #8941183 - Flags: review?(dburns) → review+
Attachment #8941183 - Attachment is obsolete: true
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/073f421c10dd
Use targetted IPC messages to talk to content frames. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/d1805bdd7b73
Sort message listeners lexicographically. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/e08b9e460de6
Refactor Marionette content frame script registration. r=automatedtester
You need to log in before you can comment on or make changes to this bug.