Closed Bug 875157 Opened 11 years ago Closed 11 years ago

BackgroundPageThumbs should block dialogs (http auth, alert(), etc.)

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: adw, Assigned: markh)

References

Details

Attachments

(2 files, 6 obsolete files)

Updated to include docs on exceptions
5.06 KB, patch
markh
: review+
Details | Diff | Splinter Review
Even more relaxed - chilled, even.
6.12 KB, patch
markh
: review+
Details | Diff | Splinter Review 
I tried setting allowAuth = false on the remote thumbnail browser's docshell, but it didn't prevent the dialog from popping up.  Does allowAuth not actually work, or does it not work for remote docshells, or what?  Other people set allowAuth = false in similar but non-remote situations, e.g., social API's FrameWorker, Add-on SDK's hidden frame.

From bug 870179:

(In reply to Drew Willcoxon :adw from comment #3)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #2)
> > (Random only vaguely related thought: how does the background thumbnail
> > service deal with pages that trigger prompts, either through e.g. alert() or
> > for HTTP auth, etc?)
> 
> [...]
> 
> I also tried capturing a page requiring authorization.  Setting allowAuth =
> false on the remote docshell in the content script didn't stop an auth
> dialog from appearing out of nowhere.  When I closed it, below it was a
> tiny, little, chromeless window containing
> chrome://global/content/mozilla.xhtml, which is used to host the thumbnail
> browser.

(In reply to Drew Willcoxon :adw from comment #4)
> Nor did hardcoding docShell->SetAllowAuth(false) in TabChild, the thought
> being that setting it right after the docshell's created might make a
> difference.  Guess allowAuth doesn't do what its name and idl comment make
> it sound like it might?
Component: General → Document Navigation
Product: Toolkit → Core
The authentication dialog here is being displayed via the HTTP channel itself (via nsHttpChannelAuthProvider::ProcessAuthentication).  This code winds up using nsIAuthPrompt2 as implemented by nsLoginManagerPrompter.jsm.  This is called as soon as the channel sees the 401 response - but it does honour the channel's LOAD_ANONYMOUS flag and avoids the prompt if this is set.

However, this flag appear to never be set as a result of nsIDocShell's allowAuth flag - which at face-value seems to be the underlying issue.  As an experiment, I added the following patch:

+++ b/docshell/base/nsDocShell.cpp
@@ -9339,16 +9339,19 @@ nsDocShell::DoURILoad(nsIURI * aURI,
         loadFlags |= nsIChannel::LOAD_INITIAL_DOCUMENT_URI;
     }

     if (mLoadType == LOAD_ERROR_PAGE) {
         // Error pages are LOAD_BACKGROUND
         loadFlags |= nsIChannel::LOAD_BACKGROUND;
     }

+    if (!mAllowAuth) {
+       loadFlags |= nsIChannel::LOAD_ANONYMOUS;
+    }

(along with setting 'docShell.allowAuth = false' in backgroundPageThumbsContent.js) and the auth-prompt was suppressed.

I guess the next step is to find someone more knowledgeable than I on this and ask if such a patch is feasible (or more information on how auth should be suppressed in this case) - any ideas on who to ask?
I'd say smaug or bz or mayhemer. However, I'm not sure that LOAD_ANONYMOUS is strictly equivalent, since it inhibits sending cookies and using any pre-existing authentication data.
Yeah, LOAD_ANONYMOUS is too blunt of a hammer (though it generally is what we want for this particular use case).

I assume we end up in nsHttpChannelAuthProvider::PromptForIdentity for triggering the actual prompt? Its behavior is to first check the channel's NotificationCallbacks, and if it can't find an nsIAuthPrompt2 there (via the complicated dance that is GetAuthPrompt()), fall back to the load group's NotificationCallbacks and try the same there.

As I understand it, the docshell sets itself as the notificationCallback for its load group. And so its mAllowAuth flag should affect the fallback behavior, when no nsIAuthPrompt2 is directly associated with the channel itself, and as far as I know this is typically the common case for docshell loads. So why is there an nsIAuthPrompt implementation associated with the channel itself, in this case?
(In reply to Mark Hammond (:markh) from comment #1)
> @@ -9339,16 +9339,19 @@ nsDocShell::DoURILoad(nsIURI * aURI,

> +       loadFlags |= nsIChannel::LOAD_ANONYMOUS;

Hmm, this gives me another idea, though. Seems like a way to do this (that isn't mAllowAuth, but some other docshell flag) would potentially help solve bug 875986.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> (In reply to Mark Hammond (:markh) from comment #1)
> > @@ -9339,16 +9339,19 @@ nsDocShell::DoURILoad(nsIURI * aURI,
> 
> > +       loadFlags |= nsIChannel::LOAD_ANONYMOUS;
> 
> Hmm, this gives me another idea, though. Seems like a way to do this (that
> isn't mAllowAuth, but some other docshell flag) would potentially help solve
> bug 875986.

I suppose the tricky bit is ensuring that behavior not only for the top-level load, but for any other loads associated with the load group - I digress.
For reasons I'm not yet sure about, the behaviour of the auth changes because we are in a remote process.  If we don't set the remote=true attribute on the browser we see the docShell itself provide the nsIAuth* interfaces and wind up with nsPrompter causing the window to open as normal.  Only with remote=true do we see the path I mentioned in comment 1.

Ironically though, if we avoid remote=true and *don't* set allowAuth=false, the nsWindowWatcher fails to open the dialog as it decides the parent window is hidden, so returns NS_ERROR_AVAILABLE at http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#640

Which got me thinking - really this bug should be broadened to something like "no modal dialogs at all" - eg, window.alert() must also fail, right?  Currently it does fail via an assertion:

0:10.52 [Child 7872] ###!!! ASSERTION: attempted to open a new window with no WindowCreator: 'mWindowCreator', file o:/src/mm/mozilla-hg/mc-socialapi-landing/embedding/components/windowwatcher/src/nsWindowWatcher.cpp, line 644
 
so maybe we are approaching this incorrectly - that we should focus on cleanly ensuring no modals at all, and no auth then falls out of that?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> I suppose the tricky bit is ensuring that behavior not only for the
> top-level load, but for any other loads associated with the load group - I
> digress.

That's also something I tried earlier, before hooking into private browsing, and couldn't get to work.  Despite storing an "anonymous" flag in the load group and propagating it to all loads in the group, I still couldn't visit pages anonymously.  (Eventually I hardcoded LOAD_ANONYMOUS in all the instances of load flags I could find.)

(In reply to Mark Hammond (:markh) from comment #6)
> so maybe we are approaching this incorrectly - that we should focus on
> cleanly ensuring no modals at all, and no auth then falls out of that?

Sure, that's the larger goal.  If it turns out that suppressing the auth dialog falls out of a blanket fix on all dialogs, great.
I'm slowly getting the hang of how all this IPC stuff works.  Once we are in a remote process, it is nsITabParent which takes on the responsibility for auth prompts etc, which is why we end up in a significantly different code path than for local tabs.  TabParent's GetAuthPrompt() says it is "largely copied" from nsIDocShell, but it doesn't check the docShell's allowAuth flag - it's not clear if that is intentional or not.

The attached patch changes this method so that it fetches the docShell for the browser's parent element and queries the allowAuth flag on that.  This means that instead of setting the flag on the browser element's docShell in the child, it needs to be done on the iframe in the parent (which is possibly why that docShell check was dropped - the semantics are different).  So this patch does prevent the auth.

I'm just uploading this for posterity - I'm not convinced it is the right thing to do as there are at least 2 other "common" prompts it doesn't suppress - proxy auth and a simple alert().  I'll investigate if it is viable to suppress *all* prompts when the parent element isn't visible (which is roughly what the window watcher service does now), in which case we will not need this patch...
Attachment #755227 - Attachment is patch: true
Attachment #755227 - Attachment mime type: text/x-patch → text/plain
(In reply to Mark Hammond (:markh) from comment #8)
> I'm slowly getting the hang of how all this IPC stuff works.  Once we are in
> a remote process, it is nsITabParent which takes on the responsibility for
> auth prompts etc, which is why we end up in a significantly different code
> path than for local tabs.  TabParent's GetAuthPrompt() says it is "largely
> copied" from nsIDocShell, but it doesn't check the docShell's allowAuth flag
> - it's not clear if that is intentional or not.
> 
> The attached patch changes this method so that it fetches the docShell for
> the browser's parent element and queries the allowAuth flag on that.  This
> means that instead of setting the flag on the browser element's docShell in
> the child, it needs to be done on the iframe in the parent (which is
> possibly why that docShell check was dropped - the semantics are different).
> So this patch does prevent the auth.

Ah, this is very illustrative - thanks. I agree that it doesn't really make sense to check the frame element's docShell for this, but perhaps we should just introduce a new way to set a flag on the TabParent (attribute on the <browser>?).

> I'm just uploading this for posterity - I'm not convinced it is the right
> thing to do as there are at least 2 other "common" prompts it doesn't
> suppress - proxy auth and a simple alert().

Proxy auth not being covered by allowAuth is an odd quirk that it seems we should change; I've been meaning to check to see why that exception was added. Alerts we could potentially deal with in other ways (e.g. using nsIDOMWindowUtils.preventFurtherDialogs).

I tend to agree that the whack-a-mole approach is not ideal, though. I wonder whether we can just neuter dialogs more globally in the IPC code - maybe in TabChild::ProvideWindow/TabChild::OpenDialog?
bz, you seem to have some blame on related code via bug 516747 - do you have any thoughts on how we could totally neuter all forms of dialogs/windows for a given remote browser?
Flags: needinfo?(bzbarsky)
Painfully.  We have a bajillion different window-opening codepaths with various different prompt providers overriding each other, the UI getting involved sometimes, etc, etc.  Short of refactoring all that code, you're going to have to hunt things down one at a time.  :(
Flags: needinfo?(bzbarsky)
This patch seems to work OK.  It stops dialogs originating in the child by calling nsIDOMWindowUtils::preventFurtherDialogs() - nsGlobalWindow checks this flag before allow modals, such as alert().  [As a side note, alert() is currently broken in child processes due to WindowWatcher asserting about the lack of a window opener - but presumably this will be fixed at some point.  This patch avoid WindowWatcher being called at all for these windows)

The patch also tries to detect hidden windows and prevent nsPrompter from displaying any prompts for such windows.  WindowWatcher does the same and uses nsWindow::IsVisible() - but this isn't exposed to javascript.  Everything I tried failed, so I gave up and just explicitly check for the window being hiddenWindow.html - which should work fine for all desktop Firefox use-cases, but possibly not in other cases.  It's this part of the patch I'm not happy with, so any advice for cleaning that up is welcome.

Includes tests for auth and alert popups.
Attachment #755783 - Flags: feedback?(adw)
(In reply to Mark Hammond (:markh) from comment #12)
> The patch also tries to detect hidden windows and prevent nsPrompter from
> displaying any prompts for such windows.  WindowWatcher does the same and
> uses nsWindow::IsVisible() - but this isn't exposed to javascript. 

We could probably just expose it, via nsIDOMWindowUtils?
Comment on attachment 755783 [details] [diff] [review]
Prevent dialogs originating from both parent and child processes

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

::: toolkit/components/prompts/src/nsPrompter.js
@@ +370,2 @@
>      if (!domWin)
> +        throw Components.Exception("openModalWindow, but no parent passed", Cr.NS_ERROR_NOT_AVAILABLE);

This seems like it might break something somewhere relying on the current behavior...  Is there a reason you can't just continue to use the activeWindow if !domWin?  This change seems unrelated to the problem, in other words.

@@ +378,5 @@
> +    let chromeWin = domWin.QueryInterface(Ci.nsIInterfaceRequestor)
> +                          .getInterface(Ci.nsIWebNavigation)
> +                          .QueryInterface(Ci.nsIDocShell)
> +                          .chromeEventHandler.ownerDocument.defaultView;
> +    if (chromeWin.location.href === 'resource://gre-resources/hiddenWindow.html')

I know your frustration! :-)

Can you try what Gavin suggests in comment 13?  Because this makes an assumption about where the thumbnail browser lives, and ideally it shouldn't do that.  (Our use of PB mode and therefore the hidden private window is in question (bug 875986).  If we go back to using the normal hidden window, then the URL on OS X won't be this.  Even if that were the case, and we checked for OS X's hidden window URL too, that's still not great because it's using the URL as a proxy for visibility and not testing visibility directly.)

::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js
@@ +63,5 @@
> +    // Arrange to prevent (most) popup dialogs for this window - popups done
> +    // in the parent (eg, auth) aren't prevented, but alert() etc are.
> +    let dwu = content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                     .getInterface(Ci.nsIDOMWindowUtils);
> +    dwu.preventFurtherDialogs();

Can you do this on init instead of every capture?

::: toolkit/components/thumbnails/test/browser_thumbnails_background.js
@@ +198,5 @@
> +  // the following tests attempt to display modal dialogs.  The test just
> +  // relies on the fact that if the dialog was displayed the test will hang
> +  // and timeout.  IOW - the tests would pass if the dialogs appear and are
> +  // manually closed by the user - so don't do that :)  (obviously there is
> +  // noone available to do that when run via tbpl etc, so this should be safe)

Hmm, could you use window watcher or somebody to watch for the opened dialog instead of relying on timing out?
Attachment #755783 - Flags: feedback?(adw) → feedback+
(In reply to Drew Willcoxon :adw from comment #14)
> ::: toolkit/components/prompts/src/nsPrompter.js
> @@ +370,2 @@
> >      if (!domWin)
> > +        throw Components.Exception("openModalWindow, but no parent passed", Cr.NS_ERROR_NOT_AVAILABLE);
> 
> This seems like it might break something somewhere relying on the current
> behavior...  Is there a reason you can't just continue to use the
> activeWindow if !domWin?  This change seems unrelated to the problem, in
> other words.

It *probably* is unrelated - however, if we do manage to wind up here without a domWin when doing thumbnails we will get a dialog - which clearly isn't what we want.  Combine this with the existing comment:

-    // XXX Investigate supressing modal state when we're called without a
-    // window? Seems odd to affect whatever window happens to be active.

I felt that we might as well try and be aggressive here.  I'll change it back if you like, but thought I'd try and make the case for trying to clean this up :)

> Can you try what Gavin suggests in comment 13?

Yep - I've got that working - I'll upload another patch after cleaning it up.

> ::: toolkit/components/thumbnails/test/browser_thumbnails_background.js
> @@ +198,5 @@
> > +  // the following tests attempt to display modal dialogs.  The test just
> > +  // relies on the fact that if the dialog was displayed the test will hang
> > +  // and timeout.  IOW - the tests would pass if the dialogs appear and are
> > +  // manually closed by the user - so don't do that :)  (obviously there is
> > +  // noone available to do that when run via tbpl etc, so this should be safe)
> 
> Hmm, could you use window watcher or somebody to watch for the opened dialog
> instead of relying on timing out?

The "normal" and correct case is obviously that no dialog is displayed.  I didn't think it worthwhile to add window-watcher code just so that if someone causes a regression in the future they see immediate failure instead of a dialog or timeout - if for no other reason that it will be difficult to test.  I believe there are *many* tests which would timeout if a regression occurs (eg, many tests would timeout if certain events don't get delivered) - is it really necessary to complicate these to handle this?
(In reply to Mark Hammond (:markh) from comment #15)
> I felt that we might as well try and be aggressive here.  I'll change it
> back if you like, but thought I'd try and make the case for trying to clean
> this up :)

I'm fine as long as it either doesn't break existing tests or if it does, those tests are fixed.

> test.  I believe there are *many* tests which would timeout if a regression
> occurs (eg, many tests would timeout if certain events don't get delivered)
> - is it really necessary to complicate these to handle this?

Yeah, I'm not saying that it's incorrect for the failure to be a timeout, just that it's less efficient: it slows down test runs (only in failing cases of course) and can make the failure harder to diagnose.  If I were writing this I'd probably use window watcher, but I wouldn't withhold an r+ if you asked me to review it with the timeout.
This patch adds a property isParentWindowMainWidgetVisible, which although verbose, is accurate - it finds the parent window's main widget and asks it if it is visible.  This is to emulate how the WindowWatcher decides if a given dialog request should be ignored due to the window being hidden.

Asking bz for review as he is already CCd on this bug, but feel free to pass it on to someone else.
Attachment #756356 - Flags: review?(bzbarsky)
An update to the previous patch which uses the isParentWindowMainWidgetVisible attribute in attachment 756356 [details] [diff] [review] and also moves preventFurtherDialogs() into init() as requested.

Try at https://tbpl.mozilla.org/?tree=Try&rev=abc3faff4437
Assignee: nobody → mhammond
Attachment #755783 - Attachment is obsolete: true
Attachment #756359 - Flags: review?(adw)
Comment on attachment 756356 [details] [diff] [review]
Add nsIDOMWindowUtils.isParentWindowMainWidgetVisible

>+    nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(docShell));

nsIDocShell inherits from nsIDocShellTreeItem, so there is no need to do this QI.

>+      if (parentTreeOwner) {

do_GetInterface is null-safe, so no need for the null-check.

But more importantly, why are we doing this on the treeowner and not directly on the docshell's GetMainWidget?

And then the function could be called isWindowWidgetVisible() or something.

The API needs to document what exceptions it throws and why, so consumers know to expect them.

I wonder how this indicates with things like alert() calls in minimized windows...  Will those claim to have a visible widget?
Attachment #756356 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #19)
> Comment on attachment 756356 [details] [diff] [review]
...
> But more importantly, why are we doing this on the treeowner and not
> directly on the docshell's GetMainWidget?

I was following the logic found in the nsWindowWatcher::OpenWindowInternal():

http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#630

I believe this code is checking the treeowner as it reflects how nsWindow::Show() was called on the top-level window.  IOW (and IIUC), it's effectively checking if the window is the hiddenDOMWindow (or other hidden windows like it that various things might create) rather than if it is minimized/off-screen.

> I wonder how this indicates with things like alert() calls in minimized
> windows...  Will those claim to have a visible widget?

Yeah, this is the reason I copied what the WindowWatcher does - I didn't want to run the risk of inventing new semantics which fail in subtle edge-cases I hadn't thought of, whereas the WindowWatcher code has been around for a long time and seems to behave as people expect.

IOW, I want this new code to block new windows whenever the WindowWatcher would have.

Does that offer enough justification for checking the treeowner, or would you prefer I still experimented with checking the docshell's GetMainWidget directly?
Flags: needinfo?(bzbarsky)
Ah, I see.  I think it's fine to match the windowwatcher behavior, but please add comments both here and in windowwatcher pointing out that the two should probably stay in sync.
Flags: needinfo?(bzbarsky)
Attached patch Updated based on feedback (obsolete) — — Splinter Review 
Updated based on feedback.  As per comment 21, we are still sticking with getting the parent's top widget and checking that.  I've added comments about the duplication of this logic to the window watcher.

I am wondering if we should change the name of the new attribute so instead of reflecting the implementation "is the parent window's main widget visible" it reflects its purpose "is the window considered visible enough to be able to open a new window" - but can't think of a name that doesn't suck (isWindowVisibleEnoughToOpenWindows or doesWindowVisibilityAllowOpeningWindows is the best I can come up with)...  I'm happy to change the name if desired.
Attachment #756356 - Attachment is obsolete: true
Attachment #756928 - Flags: review?(bzbarsky)
Comment on attachment 756928 [details] [diff] [review]
Updated based on feedback

r=me with this old comment addressed:

  The API needs to document what exceptions it throws and why, so consumers know
  to expect them.
Attachment #756928 - Flags: review?(bzbarsky) → review+
This version of the patch includes the following text in the comments in the IDL:

   * Will throw a DOM security error if called without chrome privileges or
   * NS_ERROR_NOT_AVAILABLE in the unlikely event that the parent window's
   * main widget can't be reached.

which I hope fulfills the request for docs on the exceptions.  I'm carrying the r+ forward under the assumption it does - so Drew, if you approve that other attachment I can push them both and call this done!
Attachment #756928 - Attachment is obsolete: true
Attachment #758336 - Flags: review+
Comment on attachment 756359 [details] [diff] [review]
Updated to use isParentWindowMainWidgetVisible and with other feedback addressed

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

Sorry for losing track of this.

::: toolkit/components/prompts/src/nsPrompter.js
@@ +371,5 @@
> +        throw Components.Exception("openModalWindow, but no parent passed", Cr.NS_ERROR_NOT_AVAILABLE);
> +    let winUtils = domWin.QueryInterface(Ci.nsIInterfaceRequestor)
> +                     .getInterface(Ci.nsIDOMWindowUtils);
> +    if (!winUtils.isParentWindowMainWidgetVisible)
> +        throw Components.Exception("openModalWindow for a hidden window", Cr.NS_ERROR_NOT_AVAILABLE);

Please make this error message more descriptive, like, by just adding a "cannot": "Cannot call openModalWindow on a hidden window".

::: toolkit/components/thumbnails/content/backgroundPageThumbsContent.js
@@ +14,5 @@
> +    // Arrange to prevent (most) popup dialogs for this window - popups done
> +    // in the parent (eg, auth) aren't prevented, but alert() etc are.
> +    let dwu = content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                     .getInterface(Ci.nsIDOMWindowUtils);
> +    dwu.preventFurtherDialogs();

Nit: To match the style in this file:

content.
  QueryInterface(Ci.nsIInterfaceRequestor).
  getInterface(Ci.nsIDOMWindowUtils).
  preventFurtherDialogs();

::: toolkit/components/thumbnails/test/browser_thumbnails_background.js
@@ +210,5 @@
> +
> +    let capturedURL = yield capture(url);
> +    is(capturedURL, url, "Captured URL should be URL passed to capture.");
> +    ok(file.exists(),
> +       "Thumbnail file should exist even though it requires auth.");

Please file.remove(false) here.

@@ +221,5 @@
> +
> +    let capturedURL = yield capture(url);
> +    is(capturedURL, url, "Captured URL should be URL passed to capture.");
> +    ok(file.exists(),
> +       "Thumbnail file should exist even though it alerted.");

Please file.remove(false) here.

I forgot to do that in the recently added noCookies test, so would you mind adding that?  Thanks.
Attachment #756359 - Flags: review?(adw) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/38c7ebb16a97 since master password tests weren't pleased by you.
As Drew predicted in comment 14, the stricter check which tried to insist a parent window was passed when opening a modal window was the root cause of the problem.  The prompt for the "master password" doesn't pass a window here, and best I can tell, that's somewhat close to impossible to fix.

So I just relaxed the check - we still allow a null window to be passed to the prompt service in which case we use the most-recent browser window.  If there is no most-recent window, or that window is hidden, then we still refuse to show the prompt.  This should keep the master-password code happy and it now passes all password tests locally.

This is the new patch - I'm carrying the r+ forward on it as it is 1/2 way between the existing semantics and the more aggressive semantics I tried to introduce.

Try is at https://tbpl.mozilla.org/?tree=Try&rev=f0196460a371 - I'll push when (if ;) it goes green.
Attachment #756359 - Attachment is obsolete: true
Attachment #761897 - Flags: review+
Comment on attachment 755227 [details] [diff] [review]
Query the chrome process parent element for allowAuth flag

This patch was experimental and is now obsolete.
Attachment #755227 - Attachment is obsolete: true
Comment on attachment 761897 [details] [diff] [review]
Relax requirement that the prompt service must be called with a parent window.

I don't think it's acceptable to refuse to show a dialog if there are no other windows open and no parent was passed in. My understanding is that not passing a parent window is a supported part of the nsIPromptService API (and as you've discovered people seem to rely on it), so I doesn't seem OK to be treating it as an edge case.

Of course that means we won't be able to block the prompts in question, but I think that's probably OK (I don't think it would be a problem in practice).
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #30)
> Comment on attachment 761897 [details] [diff] [review]
> Relax requirement that the prompt service must be called with a parent
> window.
> 
> I don't think it's acceptable to refuse to show a dialog if there are no
> other windows open and no parent was passed in.

I don't quite agree with that - the UX could be confusing (eg, "why is a dialog open that looks like the Firefox master password prompt when Firefox isn't running?  Am I being phished?") and may even make Firefox not work correctly (eg, what if Firefox is trying to shut down, or what if the act of opening a new window causes this prompt, making it impossible for the user to open a Firefox window while it is open?).  IMO, if the tests don't cover those edge cases, the contract is up for renewal ;)

However, I don't disagree strongly enough to try and convince anyone else!  This attachment does what Gavin requested in comment 30 and is what I just landed (the previous try run was green, and this is even more relaxed so would also pass)

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c758d2b757
https://hg.mozilla.org/integration/mozilla-inbound/rev/99fa110edd75
Attachment #761897 - Attachment is obsolete: true
Attachment #762400 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b9c758d2b757
https://hg.mozilla.org/mozilla-central/rev/99fa110edd75
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(In reply to Mark Hammond (:markh) from comment #31)
> I don't quite agree with that - the UX could be confusing (eg, "why is a
> dialog open that looks like the Firefox master password prompt when Firefox
> isn't running?  Am I being phished?") and may even make Firefox not work
> correctly (eg, what if Firefox is trying to shut down, or what if the act of
> opening a new window causes this prompt, making it impossible for the user
> to open a Firefox window while it is open?).  IMO, if the tests don't cover
> those edge cases, the contract is up for renewal ;)

Fair enough, but you need to consider that this service is used for more than just auth prompts, and other users may very well have valid reasons to show prompts when no other windows are opened. I can't think of any off-hand, but my imagination isn't as good as that of all of the other people using the platform :)
Summary: BackgroundPageThumbs should block the HTTP basic authentication dialog → BackgroundPageThumbs should block dialogs (http auth, alert(), etc.)
Depends on: 887967
Depends on: 903299
Hi,

I got the following errors thrown to the top-level of javascript interpreter
under the condtions explained at the end.

WARNING: Subdocument container has no frame: file /REF-COMM-CENTRAL/comm-central/mozilla/layout/base/nsDocumentViewer.cpp, line 2339
++DOMWINDOW == 19 (0x242529c8) [serial = 20] [outer = 0x1c52b5a8]
++DOMWINDOW == 20 (0x2e361a58) [serial = 21] [outer = 0x1c6dbf78]
++DOMWINDOW == 21 (0x250d8078) [serial = 22] [outer = 0x27208838]
++DOMWINDOW == 22 (0x2428d548) [serial = 23] [outer = 0x1c615f58]
++DOMWINDOW == 23 (0x28939658) [serial = 24] [outer = 0x1cc1beb8]
++DOMWINDOW == 24 (0x2e467838) [serial = 25] [outer = 0x251479e8]
###!!! ASSERTION: Existing entry in disk StartupCache.: 'NS_SUCCEEDED(rv) && hasEntry == false', file /REF-COMM-CENTRAL/comm-central/mozilla/startupcache/StartupCache.cpp, line 424
WARNING: cache entry deleted but not written to disk.: file /REF-COMM-CENTRAL/comm-central/mozilla/startupcache/StartupCache.cpp, line 429
JavaScript strict warning: chrome://messenger/content/tabmail.xml, line 352: reference to undefined property aTabType.panelId
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Cannot call openModalWindow on a hidden window"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///REF-OBJ-DIR/objdir-tb3/mozilla/dist/bin/components/nsPrompter.js :: openModalWindow :: line 382"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Cannot call openModalWindow on a hidden window"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///REF-OBJ-DIR/objdir-tb3/mozilla/dist/bin/components/nsPrompter.js :: openModalWindow :: line 382"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Cannot call openModalWindow on a hidden window"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///REF-OBJ-DIR/objdir-tb3/mozilla/dist/bin/components/nsPrompter.js :: openModalWindow :: line 382"  data: no]
************************************************************
GetProp:98: nbytes=32
GetProp:98: nbytes=8

I searched the source and came to this bugzilla entry.

The log is from a session  of running DEBUG BUILD Thunderbird.
(under 64bit linux), and
I am running thunderbird under valgrind (this slows it down quite a bit).

(a) - I start thunderbird under valgrind.

(b) - Since thunderbird is being tested in this test account,
  the started thunderbird offers the dialog
  to make this thunderbird the default e-mail client, newsgroup reader, 
  etc. in a modal dialog.

- this process of bringing up of modal dialog is slow under valgrind,
  so between step (a) and (b) 
  I had the time to look at the terminal window where the
  valgrind+thunderbird was started and messages were being
  printed.
 - In order to look at the terminal window, I clicked on the
   terminal window, and this hid the thunderbird main window.

I think the last hiding basically caused the situation for the
error to be thrown.


function openModalWindow(domWin, uri, args) {
    // There's an implied contract that says modal prompts should still work
    // when no "parent" window is passed for the dialog (eg, the "Master
    // Password" dialog does this).  These prompts must be shown even if there
    // are *no* visible windows at all.
    // There's also a requirement for prompts to be blocked if a window is
    // passed and that window is hidden (eg, auth prompts are supressed if the
    // passed window is the hidden window).
    // See bug 875157 comment 30 for more...
    if (domWin) {
        // a domWin was passed, so we can apply the check for it being hidden.
        let winUtils = domWin.QueryInterface(Ci.nsIInterfaceRequestor)
                             .getInterface(Ci.nsIDOMWindowUtils);
        if (winUtils && !winUtils.isParentWindowMainWidgetVisible) {
            throw Components.Exception("Cannot call openModalWindow on a hidden window",
                                       Cr.NS_ERROR_NOT_AVAILABLE);
        }
    } else {
        // We try and find a window to use as the parent, but don't consider
        // if that is visible before showing the prompt.
        domWin = Services.ww.activeWindow;
        // domWin may still be null here if there are _no_ windows open.
    }

 [...]

Usually, process (a) and (b) occur in quick succession and so nobody
seems to have realized this issue before (?)
So this is basically a race problem (?)

TIA
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to ISHIKAWA, Chiaki from comment #34)
> Hi,
> 
> I got the following errors thrown to the top-level of javascript interpreter
> under the condtions explained at the end.

I think you are looking for bug 903299.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
(In reply to Mark Hammond (:markh) from comment #35)
> (In reply to ISHIKAWA, Chiaki from comment #34)
> > Hi,
> > 
> > I got the following errors thrown to the top-level of javascript interpreter
> > under the condtions explained at the end.
> 
> I think you are looking for bug 903299.

Right, thank you. I will follow up on that bug.
Blocks: 912655
No longer blocks: 912655
Depends on: 912655
This seems to have caused a regression in dialog display which I've opened as bug 912655.

I'm surprised it hasn't been hit up to this point. It's possible I'm doing something wrong, but it's a pretty straightforward "window.openDialog" from a toolbar button that is failing.
I think this patch caused the patch in bug 847518 not to work anymore. I'll investigate to verify if this is really the cause.
Forget it, the error is triggered but the patch works anyway.
Depends on: 917610
Depends on: 924456
Hello!

I am embedding Mozilla Browser by using XulRunner and since Version 24.0 all alerts within a web page don't work anymore.

Reason:

toolkit/components/prompts/src/nsPrompter.js line 380: winUtils.isParentWindowMainWidgetVisible throws a NS_ERROR_NOT_AVAILABLE exception. (the exception is NOT thrown in line 381 where an error is returned when the window is not visible).

--------------------
367 function openModalWindow(domWin, uri, args) {
368     // There's an implied contract that says modal prompts should still work
369     // when no "parent" window is passed for the dialog (eg, the "Master
370     // Password" dialog does this).  These prompts must be shown even if there
371     // are *no* visible windows at all.
372     // There's also a requirement for prompts to be blocked if a window is
373     // passed and that window is hidden (eg, auth prompts are supressed if the
374     // passed window is the hidden window).
375     // See bug 875157 comment 30 for more...
376     if (domWin) {
377         // a domWin was passed, so we can apply the check for it being hidden.
378         let winUtils = domWin.QueryInterface(Ci.nsIInterfaceRequestor)
379                              .getInterface(Ci.nsIDOMWindowUtils);
380         if (winUtils && !winUtils.isParentWindowMainWidgetVisible) {
381             throw Components.Exception("Cannot call openModalWindow on a hidden window",
382                                        Cr.NS_ERROR_NOT_AVAILABLE);
383         }
384     } else {
385         // We try and find a window to use as the parent, but don't consider
386         // if that is visible before showing the prompt.
387         domWin = Services.ww.activeWindow;
388         // domWin may still be null here if there are _no_ windows open.
389     }
390     // Note that we don't need to fire DOMWillOpenModalDialog and
391     // DOMModalDialogClosed events here, wwatcher's OpenWindowInternal
392     // will do that. Similarly for enterModalState / leaveModalState.
393 
394     Services.ww.openWindow(domWin, uri, "_blank", "centerscreen,chrome,modal,titlebar", args);
395 }
--------------------

I've debugged this function and figured out that in line 3297 of dom/base/nsDOMWindowUtils.cpp the call to parentWindow->GetMainWidget(getter_AddRefs(parentWidget)) returns NULL.


--------------------
3279 nsDOMWindowUtils::GetIsParentWindowMainWidgetVisible(bool* aIsVisible)
3280 {
3281   if (!nsContentUtils::IsCallerChrome()) {
3282     return NS_ERROR_DOM_SECURITY_ERR;
3283   }
3284 
3285   // this should reflect the "is parent window visible" logic in
3286   // nsWindowWatcher::OpenWindowInternal()
3287   nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
3288   NS_ENSURE_STATE(window);
3289 
3290   nsCOMPtr<nsIWidget> parentWidget;
3291   nsIDocShell *docShell = window->GetDocShell();
3292   if (docShell) {
3293     nsCOMPtr<nsIDocShellTreeOwner> parentTreeOwner;
3294     docShell->GetTreeOwner(getter_AddRefs(parentTreeOwner));
3295     nsCOMPtr<nsIBaseWindow> parentWindow(do_GetInterface(parentTreeOwner));
3296     if (parentWindow) {
3297         parentWindow->GetMainWidget(getter_AddRefs(parentWidget));
3298     }
3299   }
3300   if (!parentWidget) {
3301     return NS_ERROR_NOT_AVAILABLE;
3302   }
3303 
3304   *aIsVisible = parentWidget->IsVisible();
3305   return NS_OK;
3306 }
--------------------

parentWindow was resolved to an instance of nsDocShellTreeOwner (embedding/browser/webBrowser/nsDocShellTreeOwner.cpp) and there the method GetMainWindow always returns NULL:

--------------------
647 NS_IMETHODIMP
648 nsDocShellTreeOwner::GetMainWidget(nsIWidget** aMainWidget)
649 {
650     return NS_ERROR_NULL_POINTER;
651 }
--------------------


What can i do to fix this problem?


Regards,
Dominik Hölzl
Flags: needinfo?
Depends on: 940300
(In reply to Dominik from comment #40)
> Hello!

Hello!  I opened bug 940300 for this new problem.
Flags: needinfo?
See Also: → 1188665
Hi, in the last 5-7 days, I noticed that some unit tests (xpcshell-test) in thunderbird began failing since they hit 
the assertion about mWindowCreator not null.
Googling I came to this bugzilla entry, and I was surprised that I posted a message to this bugzilla a few years ago.

E.g.: See https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=15322
pick up Windows 7 debug log.
(failed "X" test: X stands for xpcshell-test)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=15322
In the "RAW log" line, near line 15073, we have



 19:17:25     INFO -  PROCESS | 4872 | [4872] ###!!! ASSERTION: attempted to open a new window with no WindowCreator: 'mWindowCreator', file c:/builds/moz2_slave/tb-try-c-cen-w32-d-00000000000/build/mozilla/embedding/components/windowwatcher/nsWindowWatcher.cpp, line 740
 19:17:25     INFO -  PROCESS | 4872 | #01: nsWindowWatcher::OpenWindow(nsIDOMWindow *,char const *,char const *,char const *,nsISupports *,nsIDOMWindow * *) [embedding/components/windowwatcher/nsWindowWatcher.cpp:369]
 19:17:25     INFO -  PROCESS | 4872 | #02: NS_InvokeByIndex [xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:57]
 19:17:25     INFO -  PROCESS | 

Something has changed in the M-C code, I think.

I wonder if someone has any idea where to look.

TIA
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: