908277 - Make sure the background thumbnail service can't open permission UI

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Tom S [:evilpie]]

As I work on fixing all the permission UI stuff for Notifications, Geolocation etc. I have the feeling that those could actually pop up when we remote capture something? Unless of course we disable JavaScript.

Comment 1

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I don't think this is a problem, as browser.js isn't used for the <browser> element we use for remote captures (and IIUC, this is what does all the permission UI stuff)

Comment 2

[Drew Willcoxon :adw]

Attachment: ignore requests outside browser.xul

Fortunately no doorhangers appear anywhere, but a couple of exceptions are thrown because the background thumbnail browser is in the hidden window rather than browser.xul.  This one for geolocation:

[Exception... "'[JavaScript Error: "link is null" {file: "file:///Users/adw/fx/obj-debug/dist/NightlyDebug.app/Contents/MacOS/browser/components/nsBrowserGlue.js" line: 1823}]' when calling method: [nsIContentPermissionPrompt::prompt]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]

And this one for desktop-notification and pointerLock:

[Exception... "'[JavaScript Error: "chromeWin.PopupNotifications is undefined" {file: "file:///Users/adw/fx/obj-debug/dist/NightlyDebug.app/Contents/MacOS/browser/components/nsBrowserGlue.js" line: 1766}]' when calling method: [nsIContentPermissionPrompt::prompt]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]

This patch prevents the errors by returning early from ContentPermissionPrompt.prototype.prompt when the browser's host chrome window doesn't have PopupNotifications.

Comment 3

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

The null check of PopupNotifications is better than other methods of determining the window suitability because we have things like chatWindow.xul that still want to support notifications but aren't browser.xul. That probably means the comment should be tweaked, though.

I'm not sure that "get rid of the exception" is a desirable goal here, though. Throwing a better exception ("window doesn't support notifications" or somesuch) earlier on makes sense rather than letting things fail in various ways later, but it still seems like throwing an exception is the better thing to do.

Comment 4

[Drew Willcoxon :adw]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> That probably means the comment should be tweaked, though.

That makes sense.

> I'm not sure that "get rid of the exception" is a desirable goal here,
> though. Throwing a better exception ("window doesn't support notifications"
> or somesuch) earlier on makes sense rather than letting things fail in
> various ways later, but it still seems like throwing an exception is the
> better thing to do.

Do you have an idea of where such an exception should be thrown?  It looks like the prompt() method here is called directly from code in dom/, so this is the first place the call stack enters browser/, which seems to me the right place to determine whether the window supports notifications.

But I'm just trying to prevent any permission prompt exception from going uncaught, so I don't think it would be right to throw an exception with no way to catch and suppress it when you know beforehand it's not going to work.

Comment 5

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

Throwing an exception where you're currently just returning in that patch would be fine, I think. nsIContentPermissionPrompt callers should be responsible for dealing with failures accordingly.

But that won't solve your exception reporting problem, because we don't have a fix for bug 738119 yet :/ Maybe we can push on that bug.

Comment 6

[Drew Willcoxon :adw]

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

Comment 7

[Ed Morley [:emorley]]

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