Closed
Bug 1066433
Opened 10 years ago
Closed 10 years ago
Remove simple mochitest event shim
Categories
(Testing :: Mochitest, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: billm, Assigned: billm)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 3 obsolete files)
5.24 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
5.04 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
12.11 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Here's the patch to remove the old shim, already reviewed by jimm.
Attachment #8488425 -
Flags: review+
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Blocks: e10s-tests
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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•10 years ago
|
||
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.
};
Assignee | ||
Comment 7•10 years ago
|
||
New patch with fixes.
Attachment #8488432 -
Attachment is obsolete: true
Attachment #8488432 -
Flags: review?(mconley)
Attachment #8489614 -
Flags: review?(mconley)
Assignee | ||
Comment 8•10 years ago
|
||
Wrong patch.
Attachment #8489614 -
Attachment is obsolete: true
Attachment #8489614 -
Flags: review?(mconley)
Attachment #8489629 -
Flags: review?(mconley)
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c52ad7a7c3dd
https://hg.mozilla.org/mozilla-central/rev/dc407dd89b0c
https://hg.mozilla.org/mozilla-central/rev/53823b911793
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 13•10 years ago
|
||
(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?
Updated•7 years ago
|
Component: Mochitest Chrome → Mochitest
You need to log in
before you can comment on or make changes to this bug.
Description
•