191408 - Change security policy check in nsWindowWatcher::OpenWindowJS so that xpcshell can use the prompt service

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, enhancement)

Description

[timeless]

I know this sort of thing is risky, but i'd like to be able to use the
promptservice from xpcshell and at present I can't.

what i've done:
1. created a contract (@raistlin.timeless/window-creator;1) for nsWindowCreator
-- I think i've done this before (perhaps bug 131084), and I really should get
some contract into cvs so that normal xpcshell testers can have access to it.

2. patched window watcher.

Then in xpcshell, i can do this:
const C=Components.classes,I=Components.interfaces;
wc=C["@raistlin.timeless/window-creator;1"].getService(I.nsIWindowCreator2)
ww=C["@mozilla.org/embedcomp/window-watcher;1"].getService(I.nsIWindowWatcher)
ps=C["@mozilla.org/embedcomp/prompt-service;1"].getService(I.nsIPromptService)
ww.setWindowCreator(wc)
ps.alert({},"title","message")

being able to use the prompt service and other things from xpcshell is very handy.

the patch only works for modal dialogs (confirm, alert). I suspect windows would
require setting up an event loop.

Comment 1

[timeless]

Attachment: treat the lack of a script context as a sign that the script is priveleged

the scriptCX test is the only thing that fails in xpcshell. I believe that
scriptCX will not be null in browser cases so the rest of the test should
always work as expected.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 113167 [details] [diff] [review]
treat the lack of a script context as a sign that the script is priveleged

r=jst

Comment 3

[Mitchell Stoltz (not reading bugmail)]

I'm OK with this patch if jst is - in general, we assume (although it's somewhat
risky) that no context means we're in privileged mode.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 113167 [details] [diff] [review]
treat the lack of a script context as a sign that the script is priveleged

I think I meant to say sr=jst last time... sr=jst

Comment 5

[Dan M]

Comment on attachment 113167 [details] [diff] [review]
treat the lack of a script context as a sign that the script is priveleged

possible dereference of null

Comment 6

[Dan M]

Attachment: same as before but a little more

If Mitchell's happy and Johnny's happy then I'm happy. Thanks for telling me
about it. However, I think there was a flaw in the last patch. I suggest this
one. This patch has three changes: the first is from the first patch. The
second change is a drive-by patch because rv has no meaning right there so why
not a little cleanup. The third change is because secMan can possibly be null
given the first change, and while I was in there I thought it'd be nice to get
rid of some redundant NS_FAILED/object-is-null silliness.

One thing though and this wants review. This patch makes a failure to
GetSubjectPrincipal not fatal. The possibility of no script context makes this
an issue. This means the DocShellLoadInfo object will never get its Owner set.
Should this be a fatal error? I'm not certain. It is now but won't be with
these changes. I'd be comfy if Johnny and maybe Darin would take a look.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 113559 [details] [diff] [review]
same as before but a little more

I don't think we want to ignore errors from GetSubjectPrincipal(), that call
should only fail if something is really wrong, it won't fail just because
there's no context on the context stack or anything like that.

r=jst if you undo the last part of this change.

Comment 8

[timeless]

Comment on attachment 113559 [details] [diff] [review]
same as before but a little more

transfering jst's r to sr

Comment 9

[timeless]

Comment on attachment 113559 [details] [diff] [review]
same as before but a little more

marking my own r=. jst's variation works for me, so i'm committing it.

Comment 10

[timeless]

checked in, thanks