573808 - "domWin is null" error with new prompting backend and no active window

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Dave Garrett]

The new prompting backend will throw the JavaScript error "domWin is null" (from nsPrompter.js line 391) in some instances. If the given domWin is null ModalPrompter.openPrompt resorts to Services.ww.activeWindow, which would also be null on startup and possibly also when called from other odd places. (I was also getting this when called by a pref listener)

Comment 1

[Dave Garrett]

Attachment: test prompt on startup (via overlay and "load" event on window)

Here's a minimal testcase for the startup prompt error. Will give a prompt on startup under Firefox 3.6 and an error console message under current Trunk.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Looks like the only code that actually relies on a non-null domWin is dead code - fireEvent isn't called, and winUtils is unused (and removed by the WIP patch in bug 573649).

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: remove dead code

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Comment 5

[Justin Dolske [:Dolske]]

Bah, we were both working on this. Mine added a test, though. So neener-neener. ;-)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 453557 [details] [diff] [review]
Patch v.1

>diff --git a/toolkit/components/prompts/src/nsPrompter.js b/toolkit/components/prompts/src/nsPrompter.js

>     openPrompt : function (uri, args) {

>+        // XXX domWin may still be null here if there are _no_ windows open.

Don't think this is necessary (or at least not the "XXX" marker)

>diff --git a/toolkit/components/prompts/test/test_modal_prompts.html b/toolkit/components/prompts

>+// ===== test 29  =====
>+// Alert, no window
>+testNum++;
>+startCallbackTimer();
>+prompter.alert(null, "TestTitle", "This is the alert text.");
>+ok(didDialog, "handleDialog was invoked");

This doesn't actually cover the current failure case here, does it? I would expect activeWindow to be non-null when these tests are running. Doesn't hurt to add it anyways, though.

Comment 7

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/e7b4fc236003

Comment 8

[Justin Dolske [:Dolske]]

(In reply to comment #6)

> >+ok(didDialog, "handleDialog was invoked");
> 
> This doesn't actually cover the current failure case here, does it? I would
> expect activeWindow to be non-null when these tests are running. Doesn't hurt
> to add it anyways, though.

Oops, saw you comments after the push. :/

Yeah, the tests don't test this particular failure (and I'm not too sure how they could). But I did make sure they catch it if a future change removes the "if no domWin then use activeWindow" code, and it's good to have at least one case of window == null in the test.