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

RESOLVED FIXED in Firefox 51



2 years ago
2 years ago


(Reporter: jaws, Assigned: jaws)


Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox46 affected, firefox51 fixed)


MozReview Requests


Submitter Diff Changes Open Issues Last Updated
Error loading review requests:


(1 attachment, 2 obsolete attachments)

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?
Flags: needinfo?(yarik.sheptykin)
Flags: needinfo?(mdeboer)
Since the `window` object is mocked as well there, we're probably doing the wrong thing by keeping a reference at

Since only references `navigator.platform`, I'll put up a patch that simply mocks the navigator object.
Flags: needinfo?(yarik.sheptykin)
Flags: needinfo?(mdeboer)
Created attachment 8706908 [details] [diff] [review]
Patch v1: mock the navigator object for EventUtils to avoid accessing a dead object
Attachment #8706908 - Flags: feedback?(jaws)
Created attachment 8706964 [details] [diff] [review]
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.
Attachment #8706908 - Attachment is obsolete: true
Attachment #8706908 - Flags: feedback?(jaws)
Attachment #8706964 - Flags: feedback+
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") :-(
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.
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?
Flags: needinfo?(gkrizsanits)
(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).


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.
Flags: needinfo?(gkrizsanits)
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.
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.
Depends on: 1093153
Assignee: nobody → jaws
Comment hidden (mozreview-request)
Attachment #8706964 - Attachment is obsolete: true
Comment on attachment 8780884 [details]
Bug 1238065 - Fix the todo in escape_should_close_dialog by using synthesizeKey instead of sendChar.

Oooh, nice! Thanks!
Attachment #8780884 - Flags: review?(mdeboer) → review+

Comment 12

2 years ago
Pushed by
Fix the todo in escape_should_close_dialog by using synthesizeKey instead of sendChar. r=mikedeboer

Comment 13

2 years ago
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.