616779 - it isn't possible to postMessage to widget's content scripts

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

It isn't possible to post a message to a widget's content scripts, because the postMessage function is not defined.  It looks like the problem is that Widget acquires the Loader trait, whereas it should be acquiring the Symbiont trait, which itself acquires both Worker (which adds postMessage) and Loader.

(Ultimately I think this hierarchy should be flattened such that an object like Widget that wants Symbiont, Worker, and Loader functionality should acquire them all directly, and each should provide its own independent set of functionality rather than having Symbiont acquire Worker and Loader; but that's a topic for another bug.)

Comment 1

[Dietrich Ayala (:dietrich)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=569479#c15.

The problem IIRC is that Symbiont loads on construction. So we used the Loader to get the API but without loading on instanciation.

Instead of tweaking Symbiont (or Loader), we could add a postMessage to widget, and pass the message on down to each per-window Symbiont.

Comment 2

[Dietrich Ayala (:dietrich)]

Hm, while this seemed quite trivial, I'm getting an interesting error. When I call the symbiont's postMessage method, I get "this._port is null".

File "resource://testpkgs-addon-kit-tests/test-widget.js", line 513, in testPanelWidgetMessaging
    widget.postMessage(origMessage);
  File "resource://testpkgs-addon-kit-lib/widget.js", line 208, in Widget_postMessage
    browserManager.updateItem(this._public, "postMessage", message);
  File "resource://testpkgs-addon-kit-lib/widget.js", line 276, in browserManager_updateItem
    this.windows.forEach(function (w) w.updateItem(item, property, value));
  File "resource://testpkgs-addon-kit-lib/widget.js", line 276, in 
    this.windows.forEach(function (w) w.updateItem(item, property, value));
  File "resource://testpkgs-addon-kit-lib/widget.js", line 417, in BW_updateItem
    item.symbiont.postMessage(value);
  File "resource://testpkgs-api-utils-lib/content/worker.js", line 75, in postMessage
    this._port._asyncEmit('message',  JSON.parse(JSON.stringify(data)));
TypeError: this._port is null

Comment 3

[Dietrich Ayala (:dietrich)]

Irakli or Myk: What are the conditions under which a Symbiont could have a null port? From the code, it does not appear to be a valid state.

Comment 4

[Dietrich Ayala (:dietrich)]

Hm, Worker._port is only null if I try to call Worker.postMessage immediately after construction. If I wait a second, everything works as expected.

However, the Worker code looks like ._port is set immediately during construction, so I'm not sure what's going on.

Comment 5

[Dietrich Ayala (:dietrich)]

not about Worker really. the Widget content hasn't loaded yet, so Worker isn't finished setting up.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: Pointer to pull request

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 496077 [details]
Pointer to pull request

There are a few issues, which I've noted in the pull request.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

I went ahead and fixed the issues and pushed the changes in the interest of getting this into the tree.

Original commit:

https://github.com/mozilla/addon-sdk/commit/8c8bbc309428d5c7b74af18d7ceafa8bb46eb7cc

Commit to fix issues:

https://github.com/mozilla/addon-sdk/commit/f44cf76687d1d5e9942221d4ed4882249d482abe

Extraneous private merge I wish was never added to the history:

https://github.com/mozilla/addon-sdk/commit/ac77593bd1f93b1d563757bf20541c45e8cec22b

Nonextraneous public merge:

https://github.com/mozilla/addon-sdk/commit/2bc1aac18fec258b40382660457a9d5d38f20cc9

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Drew submitted this followup merge request to document the API:

https://github.com/mozilla/addon-sdk/pull/78

The merge request had one commit:

https://github.com/mozilla/addon-sdk/commit/651bff81eaf27b0b8c4b53f1fb2097e2c2935265

And I merged it in this merge commit:

https://github.com/mozilla/addon-sdk/commit/047f0e4ce9dc2db07c8a02971089bab1dfddcd0d

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Cherry-pick commits:

https://github.com/mozilla/addon-sdk/commit/25cd205d45774da75d569f55b182b440a77a4190

https://github.com/mozilla/addon-sdk/commit/624b8a24e1c7526a7a3bf788d62e37e108a13b0f

https://github.com/mozilla/addon-sdk/commit/a356b05399c027f27307c70f4bef651092772e52

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Note: I reverted the cherry-picks, as Drew and I investigated the test failures and realized that the problem is symptomatic of a potential design flaw with Widget.postMessage that will need time and deliberation to investigate and make a decision about.  More soon...

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Reverted cherry-picks:

https://github.com/mozilla/addon-sdk/commit/4e31f49a0593a75016afa786c530f5e4211aefce

https://github.com/mozilla/addon-sdk/commit/3202d4427e8eddaa8ada993796f544e6e00440df

https://github.com/mozilla/addon-sdk/commit/33d5ce22bff0b66070f5a4ca13060b098635dc05

In brief, the issue is that widget -> symbiont communication via Widget.postMessage is broadcast to all the widget's symbionts, and the test doesn't take that into account when engaging in two-way communication.  It assumes only a single symbiont will respond to its message.

Here's a prospective fix to the test:

https://github.com/mykmelez/addon-sdk/commit/9f5dd3bdd0ae8265415d205270ed65be38d8f15f

But I think we need to think more about what kinds of communication we want this API to enable.  Do we want broadcasts from a widget to its symbionts?  Or do we perhaps want only communication between a widget and a specific symbiont (the way page-mod works)?  Or both?

Comment 13

[Dietrich Ayala (:dietrich)]

Page-mods need to be able to differentiate between the symbionts. In the widget case, the symbionts map directly to windows, and I'm pretty sure (but not totally) that we want widget state to be mirrored across all windows.

I think that in the common case, the overhead of having to manage symbiont per window is extraneous to what the widget author is trying to do.

Comment 14

[Nickolay_Ponomarev]

Running the full test suite or just these two tests:
$ cfx test -b /Applications/Minefield.app -v -f "test-widget|test-windows"

fails (failure text below). This patch adds a widget.destroy() to fix the test:
<https://github.com/mozilla/addon-sdk/pull/90>

...
  File "resource://addon-kit-addon-kit-tests/test-widget.js", line 507, in 
    widget.postMessage(origMessage);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 205, in Widget_postMessage
    browserManager.updateItem(this._public, "postMessage", message);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 273, in browserManager_updateItem
    this.windows.forEach(function (w) w.updateItem(item, property, value));
  File "resource://addon-kit-addon-kit-lib/widget.js", line 273, in 
    this.windows.forEach(function (w) w.updateItem(item, property, value));
  File "resource://addon-kit-addon-kit-lib/widget.js", line 414, in BW_updateItem
    item.symbiont.postMessage(value);
  File "resource://addon-kit-api-utils-lib/content/worker.js", line 75, in postMessage
    this._port._asyncEmit('message',  JSON.parse(JSON.stringify(data)));

TypeError: this._port is null

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #14)
> This patch adds a widget.destroy() to fix the test:
> <https://github.com/mozilla/addon-sdk/pull/90>

Thanks for the test failure fix!  I have reviewed and pulled it.

Fix commit:

https://github.com/mozilla/addon-sdk/commit/2e2a26d2f9b9765b065197d2ce1b3ac5a6552a6c

Merge commit:

https://github.com/mozilla/addon-sdk/commit/8f6fffe75cc5c473fd48f7ddc6d7a6c99cf896d4

Leaving this bug open, however, pending a fix for the issue with making it possible to postMessage to a widget's content scripts.

Comment 16

[Alexandre Poirot [:ochameau]]

So the initial bug behing this bugzilla item is fixed.
Providing a window specific access to widgets is another feature discussed in bug 630962.