903299 - Modal dialog / prompt failures when running browser_thumbnails_background.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

When running browser_thumbnails_background.js on my machine I see a couple of failures that look a little worrisome:

JavaScript Error: "LoginManagerPrompter: _doAsyncPrompt:run: [Exception... "Cannot call openModalWindow on a hidden window"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: objdir-ff-release/dist/Nightly.app/Contents/MacOS/components/nsPrompter.js :: openModalWindow :: line 382"  data: no]" {file: "objdir-ff-release/dist/Nightly.app/Contents/MacOS/components/nsLoginManagerPrompter.js" line: 101}]

JavaScript error: objdir-ff-release/dist/Nightly.app/Contents/MacOS/components/nsPrompter.js, line 380: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindowUtils.isParentWindowMainWidgetVisible]

Comment 1

[Mark Hammond [:markh] [:mhammond]]

These messages come from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js#380, which is code we added explicitly to prevent auth dialogs in the b/g service.

I guess we should see if we can prevent them without making as much noise, but otherwise this is benign.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-903299-don-t-log-errors-if-the-prompter-declines.patch

This is a relatively simply patch that catches NS_ERROR_NOT_AVAILABLE exceptions and does a simple logStringMessage instead of Cu.reportError() - note there are a number of other exceptions like this thrown, so it seems to be a common pattern.  The semantics should remain identical, it just doesn't log scary errors to the console.

Comment 3

[ISHIKAWA, Chiaki]

Hi, 
I saw the same error when I ran DEBUG BUILD of thunderbird (comm-central)
under valgrind (Debian GNU/Linux 64bits/amd64).

So it would be great to grade down the superficial seriousness of the message
if the cause of the message is not that serious. (If the reason for the
disabling of modal window for a hidden window is that the program gets stuck
until the user shuffles the windows and realize there is a modal dialog 
waiting for user interaction, I agree it is a good idea to do so.
I have seen a few program installers showing modal dialog BENEATH
other windows, and so I got stuck and thought the installation
somehow stopped and failed: among such examples is the earlier VirtualBox installer,
for example.)

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 798399 [details] [diff] [review]
0001-Bug-903299-don-t-log-errors-if-the-prompter-declines.patch

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

I may be missing something, but the only openModalWindow caller is the one in this patch, and the error the patch suppresses is the one created and thrown in openModalWindow... so shouldn't we just not create that error?  And I don't think any message needs to be logged at all.

Comment 5

[ISHIKAWA, Chiaki]

(In reply to Drew Willcoxon :adw from comment #4)
> Comment on attachment 798399 [details] [diff] [review]
> 0001-Bug-903299-don-t-log-errors-if-the-prompter-declines.patch
> 
> Review of attachment 798399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I may be missing something, but the only openModalWindow caller is the one
> in this patch, and the error the patch suppresses is the one created and
> thrown in openModalWindow... so shouldn't we just not create that error? 
> And I don't think any message needs to be logged at all.

Interesting observation.

After encountering a few serious bugs last year and
crashes, I have been testing thunderbird by looking at the log messages of |make mozmill|.
There are floods of warnings/errors in the session log
that overwhelms would-be debuggers.

The problem I see is: there are shades of seriousness of warnings, errors, but the
current WARNING vs ERROR (or even ASSERTION) is not good at showing the shades of seriousness, and/or "intent" of the programmer who puts in the code to display the message.

For example, even a simple warning can mean a lot of things:

- We know that a situation handled in the code is rare, but is
  for real. To make people aware that this rare event does occur, let's print
  WARNING here when the execution hit this.

- After enumerating the possibilities mechanically, 
  we could figure out how to handle most of the possible cases.
  But we could not figure out the user interaction 
  that would lead to this particular case the code handles.
  (Imagine the handling of the final else clause of
  if-else-if-else-if-...-else.)
  For posterity, let us dump a message so that someone interested
  can figure out what has happened. (ASSERTIOON would be useful with its stack dump.)

(*)
- The processing that is put in the code is for very special case.
  We don't know exactly how often this is used, but to see
  the code is invoked and works correctly in the rare events, let us dump a message.

  Desperate kinds:

- We have written down the code for the most of the
  possible scenario. We don't know if other cases exist, and
  skipped writing code for the rest of imaginable cases since
  they are unlikely to happen.
  So if this execution path is taken, we are toast.
  Let us dump message here. Maybe an assertion is in order.
  (and we should quit right here.)

- Similar to above, but we have written down a very crude catch-all
  code that is very much inefficient. 
  We want to know if execution path reaches here, we want to know
  more details so that we can create a code to handle a particular case
  more efficiently. So let us dump message to tell people that we reach here.

- The internal logic should not allow the execution path to reach here. at all.
  Something is wrong if we hit this part, let us warn the user (and stop right here).
  ASSERTION is a likely candidate. (I would put in abort() as well to
  protect user e-mail database without proceeding further.)

 on and on.

Wading through the flotilla of warnings, errors, assertions, etc. in the
session log WITHOUT being able to figure out the original intent of the message (and seriousness associated with it) is a tiring guess work: Is the message serious, fatal, or just informative? Can we ignore, need to look into the issue more closely? 

In this sense, the particular message under discussion here seems to tell us that TB handles the case
cleverly (the type of the message marked with (*) in the above list.)

So I would think that 
- either this message can be safely eliminated, or
- to offer more intent/information by enhancing the message to be something like

  "ModalPrompter declined to show a modal ...
  =>
  "(Informational) Optimization happened: ModalPrompter declined to show a modal ...

I would prefer the latter since the message would be very instructive to the future 
developers/debuggers.
 
TIA

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-903299-don-t-log-errors-if-the-prompter-declines.patch

(In reply to Drew Willcoxon :adw from comment #4)
> Comment on attachment 798399 [details] [diff] [review]
...
> I may be missing something, but the only openModalWindow caller is the one
> in this patch, and the error the patch suppresses is the one created and
> thrown in openModalWindow... so shouldn't we just not create that error? 
> And I don't think any message needs to be logged at all.

No - you aren't missing anything - nice catch, thanks!  I simply went around in circles enough times here that I failed to realize I ended up back where I started :)

The original patch did leave one console message in the case when isParentWindowMainWidgetVisible threw an exception, which this patch now handles.  I'm inclined to agree that no message is fine, and as Chiaki also thinks that's reasonable in comment 5, this patch just silently rejects the auth prompt.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Update - forgot to commit changes!

Apologies for the bugspam!

Comment 8

[ISHIKAWA, Chiaki]

(In reply to Mark Hammond (:markh) from comment #6)
>
> The original patch did leave one console message in the case when
> isParentWindowMainWidgetVisible threw an exception, which this patch now
> handles.  I'm inclined to agree that no message is fine, and as Chiaki also
> thinks that's reasonable in comment 5, this patch just silently rejects the
> auth prompt.

Just to be sure I am not missing anything :-),
let me undersand the situation.
The current TB code wants to notify the user of the (delayed/deferred) modal dialog.
That is, TB wanted to tell the user something using a modal dialog, but for a particular hidden window, 
it was not doable (?) and the dialog could not be shown. 

Then what? 

Does TB uses another method to notify the user of this (supposedly) "IMPORTANT ISSUE" which it
originally tried to ask the user by modal dialog?
 - Is this re-asking automatically handled by the lower-level routine, or
 - if the upper-level TB code is not prepared to handle this possibility,
   the user is never asked this "IMPORTANT" question again? (This latter 
   possibility is rather bad, isn't it?)

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 799280 [details] [diff] [review]
Update - forgot to commit changes!

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

I think the question comes down to, should the prompter force every caller to handle the case where the prompt can't be shown?  Currently the answer is effectively yes, because it throws an exception in that case.  We could keep that behavior and update every caller to catch the exception.  This patch makes the answer no.

If the answer is no, then the next question is, should callers have some other way to tell when the prompt failed (e.g., via the return values of the various prompter methods)?  Then at least Thunderbird could try something else in Chiaki's example.

I think the answer to the first question leans toward no, since the app may be running code in a hidden window that's normally run in a visible window.  In that case, there's no expectation that prompts will be visible, and the app probably doesn't want them to be visible.

But there may be cases that don't conform to that reasoning, so the answer to the second question should probably be yes, but without some real-world use case that would benefit, I don't think it's necessary.

::: toolkit/components/prompts/src/nsPrompter.js
@@ +379,5 @@
>                               .getInterface(Ci.nsIDOMWindowUtils);
> +        if (winUtils) {
> +            let visible;
> +            try {
> +                visible = winUtils.isParentWindowMainWidgetVisible

Nit: no semicolon at the end of this line

@@ +384,5 @@
> +            } catch (_) {
> +                // isParentWindowMainWidgetVisible will throw
> +                // NS_ERROR_NOT_AVAILABLE an error if the main widget can't
> +                // be found - we consider that as being not visible.
> +                visible = false;

Nit: You could define visible to be false when you declare it above, and then you wouldn't need this line.

Comment 10

[Mike Kaply [:mkaply]]

I just opened bug 912655 because this problem can cause a serious failure.

I have a simple dialog that I am displaying (and there is a parent window), but I run into this problem. Although the dialog displays, because of the throw, the window.arguments values are not passed.

So this creates a regression where a dialog worked properly before the patch and doesn't work after.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw from comment #9)
> Comment on attachment 799280 [details] [diff] [review]
> Update - forgot to commit changes!
> 
> Review of attachment 799280 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the question comes down to, should the prompter force every caller
> to handle the case where the prompt can't be shown?  Currently the answer is
> effectively yes, because it throws an exception in that case.  We could keep
> that behavior and update every caller to catch the exception.  This patch
> makes the answer no.

Actually, for the alert() case the above is true, but for authentication the exception was already caught, reported and ignored.

But you raise a good point - the patch here is effectively a regression - callers of alert() would previously have seen an exception:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWindowWatcher.openWindow]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"

I don't think we should change this such that the caller doesn't see an exception.  I think we should change this to be the status quo before bug 875157 landed - just the error above being reported and the caller seeing the exception.  IOW, this patch should just arrange for the *new* message ("LoginManagerPrompter: _doAsyncPrompt:run: [Exception...") to not be reported.

Thoughts?

FWIW, I believe that once bug 875986 is fixed, we wouldn't need to take any special steps to prevent auth.  Given that bug 875157 didn't actually prevent alerts (they already didn't work due to the nsIWindowWatcher.openWindow exception above), once bug 875986 is resolved, we can probably revert both bug 875157 and the followups we make here.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Attachment: Don't log errors on failed auth, keep almost identical behaviour for alert etc.

I think this patch gives us behaviour which should be the same as we saw before bug 875157 landed (apart from no auth prompts obviously)

Before that bug, the alert() call by a background thumbnail would log:

0:06.65 JavaScript error: file:///o:/src/mozilla-git/central/obj-release/dist/bin/components/nsPrompter.js, line 394: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIWindowWatcher.openWindow]
 0:06.65 TEST-INFO | chrome://mochitests/content/browser/toolkit/components/thumbnails/test/browser_thumbnails_background.js | Console message: [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIWindowWatcher.openWindow]" {file: "file:///o:/src/mozilla-git/central/obj-release/dist/bin/components/nsPrompter.js" line: 394}]

whereas with this patch, it now logs:

0:07.31 JavaScript error: file:///o:/src/mozilla-git/central/obj-release/dist/bin/components/nsPrompter.js, line 380: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindowUtils.isParentWindowMainWidgetVisible]
 0:07.31 TEST-INFO | chrome://mochitests/content/browser/toolkit/components/thumbnails/test/browser_thumbnails_background.js | Console message: [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMWindowUtils.isParentWindowMainWidgetVisible]" {file: "file:///o:/src/mozilla-git/central/obj-release/dist/bin/components/nsPrompter.js" line: 380}]

which is effectively the same and the alert() caller sees the exception.  I don't think this patch should silence that exception - it existed before without complaint.  (While it could be argued the exception should be silenced if possible, I don't think this bug is the place to do that)

This patch just silences new errors when we decline to show an authentication dialog

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 799917 [details] [diff] [review]
Don't log errors on failed auth, keep almost identical behaviour for alert etc.

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

Sounds good.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Thanks!

try at https://tbpl.mozilla.org/?tree=Try&rev=2c01a45f1fd4

https://hg.mozilla.org/integration/fx-team/rev/b12a19b0e500

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Andreas Eibach]

(In reply to Mark Hammond [:markh] from comment #11)
> But you raise a good point - the patch here is effectively a regression -
> callers of alert() would previously have seen an exception:
> 
> [Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsIWindowWatcher.openWindow]"  nsresult: "0x80004005
> (NS_ERROR_FAILURE)"

Previously? :)
I still DO get this on FF 24.0 (X11) with an alert() message in an .init() function in one of my testing add-ons.

And I mean, I get this _instead_ of the alert() I actually wanted to see. The alert() never shows.

> I don't think we should change this such that the caller doesn't see an
> exception.

Oh yes, you should. Because IMHO it confuses the hell out of developers who thought they had done something wrong. See above.

e that once bug 875986 is fixed, we wouldn't need to take any
> special steps to prevent auth.  Given that bug 875157 didn't actually
> prevent alerts (they already didn't work due to the
> nsIWindowWatcher.openWindow exception above), once bug 875986 is resolved,
> we can probably revert both bug 875157 and the followups we make here.

It's really a horrible mess, to say the least.
I don't see through here anymore.

Comment 17

[Andreas Eibach]

Oh, sorry, wait, I misread something, get the "...modal..." one instead, like Tim above:

[Exception... "Cannot call openModalWindow on a hidden window"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"

Comment 18

[Mike Kaply [:mkaply]]

(In reply to Andreas Eibach from comment #17)
> Oh, sorry, wait, I misread something, get the "...modal..." one instead,
> like Tim above:
> 
> [Exception... "Cannot call openModalWindow on a hidden window"  nsresult:
> "0x80040111 (NS_ERROR_NOT_AVAILABLE)"

Yes, that's the error you should get and it is a bug in your code. You need to modify your code so it doesn't try to do an alert or show a dialog until there is a window available.

Comment 19

[Andreas Eibach]

Oh cone on. The alert was just for ME, i. e. for debugging purposes, not for the public! I regret to say that always used this technique in older FF versions and there were never any problems. It's a habit. :)

And if I may say so, your 'Browser Debugger' is horrible. It takes me minutes of scrolling and looking until I finally find the add-on in question. There appear to be roughly 50 content trees on the left pane, so you can imagine how long it takes until I find my add-on in the mess.

This is why some of us old-schoolers rely on the stone-age method, since it will simply save time. Well, sometimes.

Comment 20

[Mike Kaply [:mkaply]]

(In reply to Andreas Eibach from comment #19)
> Oh cone on. The alert was just for ME, i. e. for debugging purposes, not for
> the public! I regret to say that always used this technique in older FF
> versions and there were never any problems. It's a habit. :)

I agree. I ran into the same problem.

My suggestion is to print things to the console instead - Components.utils.reportError or console.log.

Comment 21

[Andreas Eibach]

(In reply to Mike Kaply (:mkaply) from comment #20)
> (In reply to Andreas Eibach from comment #19)
> > Oh cone on. The alert was just for ME, i. e. for debugging purposes, not for
> > the public! I regret to say that always used this technique in older FF
> > versions and there were never any problems. It's a habit. :)
> 
> I agree. I ran into the same problem.

Yes indeed you DID! Just found "your" bug 912655 . 
Samey :P 
Just worded a little differently. Well since you agree now, _we_ can maybe agree that they should not remove these quirks. In return, we can promise not to let these loose to the public, but for debugging it WAS invaluable. R.I.P. ;-)