Make test_nukeContentWindow.html correctly wait for the window to be destroyed instead of relying on the scheduling of the corresponding event

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
No description provided.
Assignee

Updated

2 years ago
Assignee: nobody → ehsan
Blocks: 1361461
Comment on attachment 8872089 [details] [diff] [review]
Make test_nukeContentWindow.html correctly wait for the window to be destroyed instead of relying on the scheduling of the corresponding event

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

::: js/xpconnect/tests/mochitest/test_nukeContentWindow.html
@@ +24,5 @@
> +      SpecialPowers.removeObserver(observer, "outer-window-destroyed");
> +      SpecialPowers.executeSoon(callback);
> +    }
> +  };
> +  SpecialPowers.addObserver(observer, "outer-window-destroyed");

I'd rather we use TestUtils.topicObserved for this, but if you'd prefer to keep this, I'm OK with that too. Please use an observer function rather than an object, though.
Attachment #8872089 - Flags: review?(kmaglione+bmo) → review+
Assignee

Comment 3

2 years ago
I couldn't figure out how I'm supposed to use TestUtils.jsm from a mochitest, do you know?
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #3)
> I couldn't figure out how I'm supposed to use TestUtils.jsm from a
> mochitest, do you know?

Unfortunately, you need to do something like:

    var {TestUtils} = SpecialPowers.Cu.import("resource://testing-common/TestUtils.jsm", {});
Assignee

Comment 5

2 years ago
I tried that, and that throws:

Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass

Switching to:

var {TestUtils} = SpecialPowers.unwrap(SpecialPowers.Cu.import("resource://testing-common/TestUtils.jsm", {}));

gives:

Permission denied to access property "TestUtils" at @http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_nukeContentWindow.html:21:19
Assignee

Comment 6

2 years ago
(Note that this test runs as a non-privileged mochitest-plain)
I just tested, and this works as expected:


    var {TestUtils} = SpecialPowers.Cu.import("resource://testing-common/TestUtils.jsm", {});

    ...

      yield TestUtils.topicObserved("outer-window-destroyed");

SpecialPowers.unwrap would unwrap the module global to a privileged object you have no permissions for, and unfortunately, SpecialPowers isn't smart enough to handle wrapping objects that Cu.import implicitly exports to the global, so pulling symbols off the returned wrapped module scope is the only option.

Would probably be nice to fix that...
Assignee

Comment 8

2 years ago
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #7)
> I just tested, and this works as expected:
> 
> 
>     var {TestUtils} =
> SpecialPowers.Cu.import("resource://testing-common/TestUtils.jsm", {});
> 
>     ...
> 
>       yield TestUtils.topicObserved("outer-window-destroyed");

While this works, this is the real code I need:

  yield TestUtils.topicObserved("outer-window-nuked", (subject, data) => {
    let id = subject.QueryInterface(SpecialPowers.Ci.nsISupportsPRUint64).data;
    return id == winID;
  });

Passing a function here raises the "Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass" error I mentioned before, irrespective of what's in the body of this function...

I think it's really not worth it to use this helper in this test honestly.  :-)
Ah, interesting. Yeah, probably not worth it, in that case. But your original patch didn't check the window ID. :)
Assignee

Comment 10

2 years ago
Ah yeah sorry I forgot to mention.  I hit some intermittent test failures on the try server which were being caused because the topic observer could be handling the observer notification coming from a window being nuked from a previous test run and therefore the callback would end up running too early as a result.  As a solution I'm checking the window ID to make sure the callback only handles the correct window ID.
Assignee

Comment 11

2 years ago
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #2)
> Please use an observer function rather than
> an object, though.

That also doesn't work, FWIW.  It will throw:

JavaScript error: , line 0: Error: Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass

The SpecialPowers API is unfortunately pretty unfriendly to use as far as idiomatic JS goes.

Comment 12

2 years ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8def89fd63a0
Make test_nukeContentWindow.html correctly wait for the window to be destroyed instead of relying on the scheduling of the corresponding event; r=kmag
https://hg.mozilla.org/mozilla-central/rev/8def89fd63a0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.