313753 - make window.alert throw when called from chrome

Status: REOPENED

(Core :: XUL, enhancement)

Description

[Bob Clary [:bc] (inactive)]

A discussion on irc led to bz recommending that since window.alert should not be used in chrome, we should just throw an error if anyone tries to use it in chrome. 

mconner suggests he is ok with it as along as he has a pref to turn it back on.

sideeffects could be broken extensions etc.

Comment 1

[Benjamin Smedberg]

As an alternative, why don't we make window.alert actually work correctly from chrome? We know (or can easily figure out) whether the caller is chrome or not, and we can use the "[Javascript Application]" only for content callers. It seems silly to me that we want to make the chrome/privileged window API different from the web window API.

Comment 2

[Benjamin Smedberg]

And shouldn't this really be DOM level 0?

Comment 3

[Mike Connor [:mconnor]]

I think as a best practice we want to force chrome hackers to specify titles for dialog boxes in order to plainly separate content dialogs from chrome.

window.confirm() should be brokenish too.

Comment 4

[[e-mail]]

Attachment: Basic patch

Might as well put [JavaScript Application] on all alert()s and confirm()s...

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 200757 [details] [diff] [review]
Basic patch

Yeah, that'll work.  Maybe the answer is that if it looks like untrusted content, then extension authors might not be doing the right thing otherwise.

Comment 6

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

Comment on attachment 200757 [details] [diff] [review]
Basic patch

>@@ -3324,13 +3306,7 @@ nsGlobalWindow::Prompt(const nsAString& 
> 
>   // Test whether title needs to prefixed with [script]
>   nsAutoString title;
>-  if (!IsCallerChrome()) {
>-    MakeScriptDialogTitle(aTitle, title);
>-  } else {
>-    NS_WARNING("chrome shouldn't be calling prompt(), use the prompt "
>-               "service");
>-    title.Assign(aTitle);
>-  }
>+  MakeScriptDialogTitle(aTitle, title);

That comment should probably be removed.

Comment 7

[Darin Fisher]

Doesn't this mean that we are making it easy for websites to spoof alert boxes used by extensions?  I think it makes sense for there to be a user visible distinction between app-level alert boxes and website-level alert boxes.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That brings us back to the original suggestion -- extensions should not be using alert().

Comment 9

[Jesse Ruderman]

I use alert() in extensions and other chrome code while I'm debugging.  I like being able to throw up a dialog without multiple lines of code.  I could use a pref to turn them back on, but then I wouldn't notice when shipping extensions are broken.

I also use alert() for some rare/unexpected errors in extensions I ship.  For example, Thumbs uses alert() if the user has disabled images to warn that it doesn't work well with images disabled.  If alert() is disabled for extensions, a few extensions will break in obvious ways, but many extensions will break in ways that aren't tested often.

I prefer bsmedberg's idea of making alert() work correctly from chrome.

Comment 10

[Darin Fisher]

> I prefer bsmedberg's idea of making alert() work correctly from chrome.

Which is what the current patch does, but it does so in a way that does not distinguish app-level alert from website-level alert.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Jesse, what would you consider "correct" behavior for alerts from chrome?

Comment 12

[Jesse Ruderman]

For chrome: If no title is specified, use the app name, e.g. "Firefox".  If a title is specified, use it.

For content: The current behavior, using the protocol and hostname as the title and discarding the content-provided title, seems ok.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> For chrome: If no title is specified, use the app name, e.g. "Firefox".  If a
> title is specified, use it.

You can't specify a title for alert().

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to comment #4)
> Basic patch
> 
> Might as well put [JavaScript Application] on all alert()s and confirm()s...

Note that since bug 298934 alerts etc only say "Javascript Application" for schemes that have no host, normally they show scheme and host[:port].

I think that means extension popups will show "chrome://myextension" as the title

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 200757 [details] [diff] [review]
Basic patch

sr=jst

Comment 16

[[e-mail]]

Fix checked in.

(In reply to comment #14)
>Note that since bug 298934 alerts etc only say "Javascript Application" for
>schemes that have no host, normally they show scheme and host[:port].
> 
>I think that means extension popups will show "chrome://myextension" as the title
Actually the system principal has no host because it has no uri...

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

so if chrome shouldn't be calling this, shouldn't the NS_WARNING stay in? maybe even a js console message?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Er...  I think comment 7 pointed out a major problem with that patch, no?  I'm reopening this bug, since the real problem (that alert called by chrome doesn't behave nicely) is quite unsolved.

Comment 19

[[e-mail]]

Attachment: Block chrome window alerts?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Mm... Is the goal to prevent alert() being called on a chrome window, or by chrome code?  Or both?

Comment 21

[Boris Zbarsky [:bzbarsky]]

In particular, non-chrome code can run in chrome windows (eg in chatzilla).

Comment 22

[Benjamin Smedberg]

I really don't think we ought to be walking down this path of blocking window.alert. Admittedly chrome is not content, but window.alert is an API that has been around since the dawn of time. Let's fix it to have a good default title for chrome alerts (the appname, or even "Extension Alert" would be good enough for me).

Comment 23

[James Ross]

Could we not, technically, manage to work out *which* extension it is?

I would say that logging a warning to the JS Console when chrome calls it would be good enough, but Firefox has bad defaults for that, though it may still be OK (if we assume extension devs are all smart enough to change the prefs).

If it does get "blocked", it absolutely must point the author at the prompt service.

Comment 24

[Johnny Stenback (:jst)]

(In reply to comment #22)
> I really don't think we ought to be walking down this path of blocking
> window.alert. Admittedly chrome is not content, but window.alert is an API that
> has been around since the dawn of time. Let's fix it to have a good default
> title for chrome alerts (the appname, or even "Extension Alert" would be good
> enough for me).
> 

I really don't get the reasoning behind this either. Make alert() when called from chrome somehow indicate that etc, fine, but making it throw an exception seems a step in the exact wrong direction IMNSHO.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200863 [details] [diff] [review]
Block chrome window alerts?

sr-.  And we should probably consider backing out that first patch too....