1068087 - Add a simple mechanism for content pages to communicate with chrome

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Dave Townsend [:mossop]]

The main process should be able to say that it is interested in talking to specific URLs loaded in the content process. Any page at that URL then gets addMessageListener/sendAsyncMessage functions that pass through to chrome without needing a content script.

This is somewhat similar to WebChannel but uses a more direct API instead of relying on custom event dispatch.

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: bz://1068087/Mossop

/r/4319 - Bug 1068087: Add a simple mechanism for content pages to communicate with chrome.
/r/4419 - Switch about:plugins to run remotely.

Pull down these commits:

hg pull review -r ddd65219f09a5e569879f8a209b9b3860fefa85b

Comment 2

[Dave Townsend [:mossop]]

Too me about a bajillion years longer than I had planned to get this finished but here it is at least. One changeset to add the API, one to put it to use for about:plugins.

Comment 3

[Mike de Boer [:mikedeboer]]

This looks like an even better API to use than WebChannels for about:loop* pages! (to replace navigator.mozLoop)

Comment 4

[Dave Townsend [:mossop]]

Note that a follow-up to this bug should be to add support for using a RegExp to select pages, not just complete string matching.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8570295 [details]
MozReview Request: bz://1068087/Mossop

https://reviewboard.mozilla.org/r/4317/#review3629

This is pretty epic! I've got a few questions / notes - see below.

Also, this is going to need some MDN documentation, I think. Examples for people to look at. And maybe a mailing list post once this thing ships, since I think people are going to find it pretty handy.

::: toolkit/content/widgets/browser.xml
(Diff revision 1)
> +          // Give others a chance to swap state.
> +          let event = new CustomEvent("SwapDocShells", {"detail": aOtherBrowser});
> +          this.dispatchEvent(event);
> +          event = new CustomEvent("SwapDocShells", {"detail": this});
> +          aOtherBrowser.dispatchEvent(event);

Yeah, this makes more sense in here.

::: toolkit/content/browser-content.js
(Diff revision 1)
> +new PageListener(this);

I would love some documentation right here describing what PageListener is used for in the scope of web content.

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +    Services.mm.addMessageListener("RemotePage:InitListener", this.listenerInit.bind(this));
> +    Services.mm.addMessageListener("RemotePage:InitPort", this.portInit.bind(this));

Nit - InitListener calls listenerInit, InitPort calls portInit... maybe for consistency, we could callt hese functions initListener, initPort.

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +  Services.obs.addObserver(observer, "chrome-document-global-created", false);
> +  Services.obs.addObserver(observer, "content-document-global-created", false);

I guess this is a no-op for repeated instantiation of PageListener, and we don't need to worry about de-registering these observers?

Also, for the very first remote chrome document, I guess the chrome-document-global-created notification will fire _after_ this has observer has been set?

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +  let portID = Services.appinfo.processID + ":" + ChildMessagePort.prototype.nextPortID++;

ChildMessagePort.prototype.nextPortID++... isn't that just this.nextPortID++?

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> + * mnsIMessageListenerManager

typo: nsIMessageListenerManager

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +function MessagePort(messageManager, portID) {

I guess both ChromeMessagePort and ContentMessagePort "inherit" from this? Once bug 837314 gets fixed and we can use it in core, I think we're all going to be a lot happier.

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +   *   target: This message port

You seem to use this pattern a lot for your callbacks:

```
({target: port}) => {
  // Do something with port
}
```

Why not just name it port?

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> + * messages from all loaded pages.

"all loaded pages [from the requested url]", or something to that effect.

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +  this.removeMessagePort = this.removeMessagePort.bind(this);
> +  this.portMessageReceived = this.portMessageReceived.bind(this);

Forgive my ignorance, but why are you finding this necessary? What are you guarding against with those binds?

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +    data: null

Nit - please add trailing comma

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +      data: data

Nit - please add trailing comma

::: toolkit/modules/RemotePageManager.jsm
(Diff revision 1)
> +  // Add functionality to the content page
> +  Cu.exportFunction(this.sendAsyncMessage.bind(this), window, {
> +    defineAs: "sendAsyncMessage",
> +  });
> +  Cu.exportFunction(this.addMessageListener.bind(this), window, {
> +    defineAs: "addMessageListener",
> +    allowCallbacks: true,
> +  });
> +  Cu.exportFunction(this.removeMessageListener.bind(this), window, {
> +    defineAs: "removeMessageListener",
> +    allowCallbacks: true,
> +  });

We're creating a ChildMessagePort when we see the content-document-global-created observer notification... so does that means these functions are going to be exposed to the global scope of any random web content that matches one of our URLs?

::: toolkit/modules/tests/browser/browser_RemotePageManager.js
(Diff revision 1)
> +

Might be worth testing that you can communicate with port1 now that the page for port2 has closed.

::: toolkit/modules/tests/browser/browser_RemotePageManager.js
(Diff revision 1)
> +  if (pongPorts[0] == port1) {
> +    is(pongPorts[0], port1, "Should have received pongs from the right ports");
> +    is(pongPorts[1], port2, "Should have received pongs from the right ports");
> +  }
> +  else {
> +    is(pongPorts[0], port2, "Should have received pongs from the right ports");
> +    is(pongPorts[1], port1, "Should have received pongs from the right ports");
> +  }

I guess you're dealing with some non-determinism here? Can you explain that non-determinism?

::: toolkit/modules/tests/browser/testremotepagemanager.html
(Diff revision 1)
> +function buttonClicked() {
> +  sendAsyncMessage("ButtonClicked");
> +}

What's this for?

::: toolkit/modules/tests/browser/testremotepagemanager.html
(Diff revision 1)
> +  <button onclick="buttonClicked()">Send Message</button>

Needed?

::: toolkit/modules/tests/browser/browser_RemotePageManager.js
(Diff revision 1)
> +const TEST_URL2 = "http://www.example.com/browser/toolkit/modules/tests/browser/testremotepagemanager2.html";

This is never used.

Comment 6

[Justin Dolske [:Dolske]]

FWIW, UITour also has a mechanism that's vaguely(?) similar to what this bug is. [See content-UITour.js as a starting point.] Might be worth considering if UITour should move to this mechanism at some point?

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8570295 [details]
MozReview Request: bz://1068087/Mossop

/r/4319 - Bug 1068087: Add a simple mechanism for content pages to communicate with chrome.
/r/4419 - Switch about:plugins to run remotely.

Pull down these commits:

hg pull review -r 6ecd357b5f9966a42ae1342f48b81fd338fc1fe7

Comment 8

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/4317/#review3725

Yeah I want to do some docs and blog posts about both this and bug 1130529

> Forgive my ignorance, but why are you finding this necessary? What are you guarding against with those binds?

The first can be dropped, portMessageReceived though is used as a callback for addMessageListener so we need to bind so "this" is defined correctly.

> I guess both ChromeMessagePort and ContentMessagePort "inherit" from this? Once bug 837314 gets fixed and we can use it in core, I think we're all going to be a lot happier.

So much happier

> You seem to use this pattern a lot for your callbacks:
> 
> ```
> ({target: port}) => {
>   // Do something with port
> }
> ```
> 
> Why not just name it port?

I felt that it was useful to maintain consistency with the nsIMessageListener API, I'm open to be argued with though.

> ChildMessagePort.prototype.nextPortID++... isn't that just this.nextPortID++?

Nope! Here we want a global counter that increases for every ChildMessagePort created in this process so portID is guaranteed unique. Now here is some JS fun. "foo++" is the same as saying "foo = foo + 1". So "this.nextPortID++" would be "this.nextPortID = this.nextPortID + 1". this.nextPortID doesn't exist at that point so defers to the prototype but assigning the value to this.nextPortID actualyl sets the property on this directly, it won't alter the prototype value. So ChildMessagePort.prototype.nextPortID never changes and evey ChildMessagePort ends up with ID 1. Because JS.

if it makes things clearer I could just define nextPortID as a top-level variable.

> We're creating a ChildMessagePort when we see the content-document-global-created observer notification... so does that means these functions are going to be exposed to the global scope of any random web content that matches one of our URLs?

Correct, that's basically the point. Initially we should only use this for internal pages that webpages can't link to, other uses would need a security review. The exposure is only the ability to send messages to the listener in the chrome process but depending on what that can do that could be bad.

> I guess this is a no-op for repeated instantiation of PageListener, and we don't need to worry about de-registering these observers?
> 
> Also, for the very first remote chrome document, I guess the chrome-document-global-created notification will fire _after_ this has observer has been set?

It shouldn't be a no-op, every PageListener will see every document that loads in its process. That's why the observer needs to verify the notification isn't for a page in a different frame. Now we have process scripts it might make more sense to switch to creating PageListener in one of those instead, I seem to recall I had some problems why I tried to do this globally though so I wanted to look at that in a follow-up.

And yes my understanding is that frame scripts come up before the frame starts to load anything.

Comment 9

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #6)
> FWIW, UITour also has a mechanism that's vaguely(?) similar to what this bug
> is. [See content-UITour.js as a starting point.] Might be worth considering
> if UITour should move to this mechanism at some point?

UITour would be better suited to WebChannel as it handles the permission manager checking for you. It would be nice if WebChannel used this though.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8570295 [details]
MozReview Request: bz://1068087/Mossop

This latest revision appears to break about:plugins for both e10s and non-e10s windows. Mossop said he'd investigate - clearing review request for now.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8570295 [details]
MozReview Request: bz://1068087/Mossop

/r/4319 - Bug 1068087: Add a simple mechanism for content pages to communicate with chrome.
/r/4419 - Switch about:plugins to run remotely.

Pull down these commits:

hg pull review -r 27aaca191c5546dbecf32c52e2226b67183f0082

Comment 12

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/4319/#review3759

Ship It!

Comment 13

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/4419/#review3761

Ship It!

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8570295 [details]
MozReview Request: bz://1068087/Mossop

https://reviewboard.mozilla.org/r/4317/#review3763

This looks good to me - great work! I'm looking forward to seeing the MDN docs, and blog / mailing list posts. :D

Comment 15

[Dave Townsend [:mossop]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/6dc164972d9a
remote:   https://hg.mozilla.org/integration/fx-team/rev/0718dffcd078

Comment 16

[Carsten Book [:Tomcat]]

backed this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2148673&repo=fx-team

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 8570295 [details]
MozReview Request: bz://1068087/Mossop

/r/4319 - Bug 1068087: Add a simple mechanism for content pages to communicate with chrome. r=mconley
/r/4419 - Bug 1068087: Switch about:plugins to run remotely. r=mconley

Pull down these commits:

hg pull review -r 02e6199e5e8d416067317612270132fe791fd2f8

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8570295 [details]
MozReview Request: bz://1068087/Mossop

https://reviewboard.mozilla.org/r/4317/#review4331

Assuming a green try run, r=me. I'm excited to get this into the tree - it's going to make converting our about: pages much easier. :)

Great work on this!

::: toolkit/content/process-content.js
(Diff revision 4)
> +let { classes: Cc, interfaces: Ci, utils: Cu } = Components;

We don't really need Cc and Ci, do we?

Comment 19

[Dave Townsend [:mossop]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/1b3657fee5a8
remote:   https://hg.mozilla.org/integration/fx-team/rev/9393c7434d4e

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1b3657fee5a8
https://hg.mozilla.org/mozilla-central/rev/9393c7434d4e

Comment 21

[Marco Mucci [:MarcoM]]

Hi Dave, can you provide a point value.

Comment 22

[Mike de Boer [:mikedeboer]]

Dave, I agree with Mike: Great work!

Can you post the doc/ blog links in here for annotation purposes? Thanks!

Comment 23

[Dave Townsend [:mossop]]

Good idea, I could do with more traffic to my blog ;)

http://www.oxymoronical.com/blog/2015/03/Making-communicating-with-chrome-from-in-content-pages-easy
https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/RemotePageManager

Comment 24

[:Gijs (he/him)]

Can you also update https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Services.jsm ? I would do it myself but I'm not sure what the cpmm/ppmm things are and where they should link...

Comment 26

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1068087: Switch about:plugins to run remotely. r=mconley

Comment 27

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1068087: Add a simple mechanism for content pages to communicate with chrome. r=mconley