Closed Bug 1068087 Opened 5 years ago Closed 5 years ago

Add a simple mechanism for content pages to communicate with chrome

Categories

(Toolkit :: General, defect)

defect
Not set
Points:
8

Tracking

()

RESOLVED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox39 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

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.
Attached file MozReview Request: bz://1068087/Mossop (obsolete) —
/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
Attachment #8570295 - Flags: review?(mconley)
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.
This looks like an even better API to use than WebChannels for about:loop* pages! (to replace navigator.mozLoop)
Blocks: 1048850
Status: NEW → ASSIGNED
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 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.
Attachment #8570295 - Flags: review?(mconley)
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 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
Attachment #8570295 - Flags: review?(mconley)
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.
(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 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.
Attachment #8570295 - Flags: review?(mconley)
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
Attachment #8570295 - Flags: review?(mconley)
Attachment #8570295 - Flags: review?(mconley) → review+
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
backed this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2148673&repo=fx-team
Flags: needinfo?(dtownsend)
Depends on: 1139628
Depends on: 1141661
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
Attachment #8570295 - Flags: review+ → review?(mconley)
Flags: needinfo?(dtownsend)
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?
Attachment #8570295 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/1b3657fee5a8
https://hg.mozilla.org/mozilla-central/rev/9393c7434d4e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Hi Dave, can you provide a point value.
Iteration: --- → 39.2 - 23 Mar
Flags: qe-verify?
Flags: needinfo?(dtownsend)
Flags: firefox-backlog+
Points: --- → 8
Flags: needinfo?(dtownsend)
Flags: qe-verify? → qe-verify-
Dave, I agree with Mike: Great work!

Can you post the doc/ blog links in here for annotation purposes? Thanks!
Flags: needinfo?(dtownsend)
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...
Flags: needinfo?(dtownsend)
Keywords: dev-doc-needed
Flags: needinfo?(dtownsend)
Attachment #8570295 - Attachment is obsolete: true
Attachment #8618370 - Flags: review+
Attachment #8618371 - Flags: review+
You need to log in before you can comment on or make changes to this bug.