1238065 - browser_subdialogs.js escape_should_close_dialog test disabled due to "can't access dead object" on `navigator` via BrowserTestUtils.sendChar

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

In the test changes introduced by bug 1087114, this code:

> add_task(function* escape_should_close_dialog() {
>   yield open_subdialog_and_test_generic_start_state(tab.linkedBrowser);
> 
>   info("canceling the dialog");
>   yield close_subdialog_and_test_generic_end_state(tab.linkedBrowser,
>     function() { return BrowserTestUtils.sendChar("VK_ESCAPE", tab.linkedBrowser); },
>     null, undefined, {runClosingFnOutsideOfContentTask: true});
> });

causes "can't access dead object" when referencing `navigator`. Iaroslav or Mike, do you have any ideas why this might be happening?

Comment 1

[Mike de Boer [:mikedeboer]]

Since the `window` object is mocked as well there, we're probably doing the wrong thing by keeping a reference at https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/AsyncUtilsContent.js#15

Since https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js only references `navigator.platform`, I'll put up a patch that simply mocks the navigator object.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: mock the navigator object for EventUtils to avoid accessing a dead object

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1 + tests

Thanks Mike. I re-enabled the test that was disabled, and that will include code coverage for this now.

However, this is not enough. Your change here got rid of the dead object, but the code stalls out a few lines later with the error:

121 INFO Console message: [JavaScript Error: "TypeError: KeyboardEvent is not a constructor" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 1236}]

It looks like KeyboardEvent will have to be mocked out as well.

Comment 4

[Mike de Boer [:mikedeboer]]

I saw that too just a while ago, but I do not have a clue why it's not a constructor. I don't think I should mock KeyboardEvent, because I think the window _needs_ a real event object to propagate it.

When I made sure that KeyboardEvent a constructor (fixing this error), it didn't work! IOW, it didn't close the dialog with the sendChar("VK_ESCAPE") :-(

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

The other question though is why are these being killed in the first place. The document isn't going away mid-test, so they shouldn't be dead yet.

Comment 6

[Mike de Boer [:mikedeboer]]

So I've been thinking about this, since I've been spending lots more time with EventUtils.js in combination with frame scripts.
What I'm suspecting here is that the sub-script loader has different memory management semantics than imported module scripts. The trick we do here is that we assign variables that we want to appear as 'global' when the EventUtils.js script is evaluated in the frame script.
I wouldn't be surprised if a condition existed that makes the GC do a marking pass on the sub-script tree of global variables.
The question is if there is a difference in GC behavior between frame-scripts, module scripts and sub-scripts?

I don't know the answer, but I can think of two solutions:
1) JS modules - `Cu.import("EventUtils.jsm")` - appear to behave more reliably and passing objects between frame-scripts and modules seems to _not_ be problematic. I've been working on making EventUtils.js load-able as a module in bug 1154277 so that it may be used in ContentTasks as well. When that work is done, we can also remove AsyncUtilsContent.js and move everything over to using ContentTask.
All this work should probably move to separate bugs, not be part of the Hello project.
2) Find someone to fix the sub-script loaders' GC semantics. Something like `loadSubScriptWithOptions("EventUtils.js", { loadAsModule: true });`. Or something different, better :-)

Gabor, do you - or someone else you can think of - know if there's a difference in GC behavior as I described above?

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> Gabor, do you - or someone else you can think of - know if there's a
> difference in GC behavior as I described above?

I think the main difference is that jsm's are loaded and evaluated only once per process, and then every subsequent call only exports the properties the module exports to the new global, while subscript loader loads and evals each time you load the same script to a new global (there is a precompile option for it though, but the eval still occurs again by each call).

See: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/idl/xpcIJSModuleLoader.idl#45
and: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSSubScriptLoader.cpp#520

Finally frame scripts are a bit like subscripts, they are executed in each frame's scope once. There is also the process script that is a bit like jsm's in a sense that they are executed only once per process.

Comment 8

[Mike de Boer [:mikedeboer]]

Thanks Gabor! I'm attempting to fix this in a more general way in bug 1241532; make EventUtils available in the frame's scope and use Cu.import for that to extend its lifecycle as much as possible.

Comment 9

[Mike de Boer [:mikedeboer]]

Hmm, instead, I came across this issue myself in bug 1093153 and I'm doing a different type of fix there (Patch 1).
Direction I'm taking there:

1) Avoid using `navigator` global object in general inside EventUtils.js
2) use `content.KeyboardEvent` when the global `KeyboardEvent` can not be used as a constructor, which is the case in this bug too.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1238065 - Fix the todo in escape_should_close_dialog by using synthesizeKey instead of sendChar.

Review commit: https://reviewboard.mozilla.org/r/71456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71456/

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8780884 [details]
Bug 1238065 - Fix the todo in escape_should_close_dialog by using synthesizeKey instead of sendChar.

https://reviewboard.mozilla.org/r/71456/#review69062

Oooh, nice! Thanks!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3c089287bf6
Fix the todo in escape_should_close_dialog by using synthesizeKey instead of sendChar. r=mikedeboer

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e3c089287bf6