1066433 - Remove simple mochitest event shim

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Right now we have a simple shim that forwards load, DOMContentLoaded, and pageshow events from the child to the parent when running mochitests. However, browser-chrome tests now run using the same shims that add-ons get. Therefore, our add-on shims should also cover our tests. And the add-on shims have a much nicer way of forwarding these events.

Originally I posed a patch for removing the simple shim in bug 1059007 (which Jim reviewed). However, getting it to work has been more effort than I expected, so I'm splitting it out into a separate bug.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remove-simple-event-shim

Here's the patch to remove the old shim, already reviewed by jimm.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: shim-notifications

This patch fixes some bugs in the new shims. I realized that the code that decides whether to register or unregister a listener in the child is broken. There are two problems.

1. When a new child starts up, we send all the topics it's supposed to listen on over to it, including the number of listeners registered in the parent. However, the registration code simply says |if (listener_count == 1) { register(); } else if (listener_count == 0) { unregister(); }|. It assumes that the listener count will only be incremented or decremented by one. But a new child that's just starting up may see the count immediately go to some value like 4. In that case, we don't register anything.

2. Some listeners (like event listeners) are tab-specific. In that case, the |if (listener_count == 1) { register(); } else ...| code is even more broken since it says nothing about individual tabs, even though each one needs to register separately.

To fix this, I created a table that records whether we've registered a listener for a given topic yet. This table is specific to a given shim. For tab-specific shims, we'll have a separate table for each tab. I think this should solve all the problems.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: handle-null-content

A lot of tests do this:

let tab = gBrowser.addTab();
tab.linkedBrowser.contentWindow.location = foo;

In e10s, we haven't yet received a CPOW for the contentWindow at this time, so that will be null. To work around this, I made the contentWindow shim return a dummy object in that case. The dummy object has a setter for location and nothing else.

This could be a problem if someone saves contentWindow for later, since they'll still get the dummy object. But it's better than returning null, I think.

I also did a similar thing for contentDocument.readyState, which I saw a test using very early.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: handle-null-content

I just ran into a test that looks at contentDocument on the <tabbrowser>, so I added a case for that.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8488425 [details] [diff] [review]
remove-simple-event-shim

Review of attachment 8488425 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/mochitest-e10s-utils.js
@@ +7,2 @@
>  // care about and so doesn't normally get special support, eg, the "pageshow"
>  // or "load" events).

it looks like some of these comments should go too

Comment 6

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

Comment on attachment 8488432 [details] [diff] [review]
shim-notifications

Review of attachment 8488432 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +82,3 @@
>  
> +  watch: function(component1, watcher) {
> +    setDefault(this._watchers, component1, []).push(watcher);

So now we're pushing the watcher object into this _watchers dict, and I don't see us ever clearing out those watchers.

I'm also seeing us set the watcher as the key of a WeakMap.

By holding onto a ref to the watcher in this._watchers, haven't we guaranteed that the watcher will never be GC'd? Should we be removing watchers somewhere? If not, then why a WeakMap?

@@ +82,4 @@
>  
> +  watch: function(component1, watcher) {
> +    setDefault(this._watchers, component1, []).push(watcher);
> +    this._registered.set(watcher, {});

Out of curiosity, why a {}, and not a Map, which seems more suited to the job of storing "pathString" -> "true/nothing"? Or, maybe better, a Set, which just contains pathStrings of registered items?

@@ +88,1 @@
>      function enumerate(tracked, curPath) {

If you want to avoid the self business:

let enumerate = (tracked, curPath) => {
  // "this" in here will be the outer this.
};

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: shim-capturing-fix v2

New patch with fixes.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: shim-notifications v2

Wrong patch.

Comment 9

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

Comment on attachment 8489629 [details] [diff] [review]
shim-notifications v2

Review of attachment 8489629 [details] [diff] [review]:
-----------------------------------------------------------------

Looks sane to me - thanks billm.

::: toolkit/components/addoncompat/RemoteAddonsChild.jsm
@@ +46,3 @@
>      let [paths] = cpmm.sendSyncMessage("Addons:GetNotifications");
>      this._paths = paths;
> +    this._registered = new WeakMap();

Since we're doing the watching / unwatching stuff, does this still need to be a WeakMap?

Comment 10

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

Comment on attachment 8488440 [details] [diff] [review]
handle-null-content

Review of attachment 8488440 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +656,3 @@
>  RemoteBrowserElementInterposition.getters.contentDocument = function(addon, target) {
> +  // If we don't have a CPOW yet, just return something we can use to
> +  // example readyState. This is useful for tests that create a new

"example" -> "sample"?

::: toolkit/components/addoncompat/multiprocessShims.js
@@ +88,5 @@
>  
>      const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +    if (target instanceof Ci.nsIDOMXULElement) {
> +      if (target.localName == "browser" &&
> +          target.getAttribute("remote") == "true") {

Alternatively, target.isRemoteBrowser.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c52ad7a7c3dd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dc407dd89b0c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/53823b911793

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c52ad7a7c3dd
https://hg.mozilla.org/mozilla-central/rev/dc407dd89b0c
https://hg.mozilla.org/mozilla-central/rev/53823b911793

Comment 13

[Jim Mathies [:jimm]]

(In reply to Bill McCloskey (:billm) from comment #3)
> Created attachment 8488434 [details] [diff] [review]
> handle-null-content
> 
> A lot of tests do this:
> 
> let tab = gBrowser.addTab();
> tab.linkedBrowser.contentWindow.location = foo;
> 
> In e10s, we haven't yet received a CPOW for the contentWindow at this time,
> so that will be null. To work around this, I made the contentWindow shim
> return a dummy object in that case. The dummy object has a setter for
> location and nothing else.
> 
> This could be a problem if someone saves contentWindow for later, since
> they'll still get the dummy object. But it's better than returning null, I
> think.
> 
> I also did a similar thing for contentDocument.readyState, which I saw a
> test using very early.

Maybe in debug builds the return of this temporary object should warn through the console, so devs know they are getting something funky?