Closed Bug 1066433 Opened 7 years ago Closed 7 years ago

Remove simple mochitest event shim

Categories

(Testing :: Mochitest, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: billm, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.
Here's the patch to remove the old shim, already reviewed by jimm.
Attachment #8488425 - Flags: review+
Attached patch shim-notifications (obsolete) — Splinter Review
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.
Attachment #8488432 - Flags: review?(mconley)
Attached patch handle-null-content (obsolete) — Splinter Review
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.
Attachment #8488434 - Flags: review?(mconley)
I just ran into a test that looks at contentDocument on the <tabbrowser>, so I added a case for that.
Attachment #8488434 - Attachment is obsolete: true
Attachment #8488434 - Flags: review?(mconley)
Attachment #8488440 - Flags: review?(mconley)
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 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.
};
Attached patch shim-capturing-fix v2 (obsolete) — Splinter Review
New patch with fixes.
Attachment #8488432 - Attachment is obsolete: true
Attachment #8488432 - Flags: review?(mconley)
Attachment #8489614 - Flags: review?(mconley)
Wrong patch.
Attachment #8489614 - Attachment is obsolete: true
Attachment #8489614 - Flags: review?(mconley)
Attachment #8489629 - Flags: review?(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?
Attachment #8489629 - Flags: review?(mconley) → review+
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.
Attachment #8488440 - Flags: review?(mconley) → review+
Blocks: 1084637
(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?
Component: Mochitest Chrome → Mochitest
You need to log in before you can comment on or make changes to this bug.