384014 - strange js error "window is not defined"

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[georgi - hopefully not receiving bugspam]

Attachment: unload4e.html

"window is not defined is strange js error.

when unload4e.html is opened, the link is clicked and then |reload| is clicked there is a
strange error in js console:

Error: window is not defined
Source File: {window.open(location,"_self")}
Line: 1

Comment 1

[Martijn Wargers (dead)]

Why do you think this should be security sensitive?

Comment 2

[georgi - hopefully not receiving bugspam]

well i suspect this may be exploitable - if there is no window probably there is no document so the js principal may not be found.

feel free to remove the security flag.

Comment 3

[Martijn Wargers (dead)]

Well, I was just asking, it's not directly obvious of why it should be security sensitive, with the initial bug report.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is every single piece of the testcase needed?

Blake, you want to check this out?  I don't see how our JS scope is getting that confused...

Comment 5

[georgi - hopefully not receiving bugspam]

(In reply to comment #4)
> Is every single piece of the testcase needed?
> 

not quite sure, but have troubles minimizing it more...

Comment 6

[georgi - hopefully not receiving bugspam]

adding |debugger| before window.open shows the script is in the sandbox:

Hit JavaScript "debugger" keyword. JS call stack...
0 <TOP LEVEL> ["{debugger;window.open(location,"_self")}":1]
    this = [object Sandbox]

so this is not scary?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh.  No, running javascript: URIs that are running against some random scope they shouldn't be running against in a sandbox is what we want to be doing.  And of course there's no |window| in there.

Comment 8

[georgi - hopefully not receiving bugspam]

(In reply to comment #7)
> Oh.  No, running javascript: URIs that are running against some random scope
> they shouldn't be running against in a sandbox is what we want to be doing. 
> And of course there's no |window| in there.
> 

so this bug is invalid?

Comment 9

[georgi - hopefully not receiving bugspam]

since this is not in the sandbox on branch should the defined window of branch be examined?

Comment 10

[Boris Zbarsky [:bzbarsky]]

It might be worth figuring out when it started happening in the sandbox on trunk, at least...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Oh, and it's not obvious that we _should_ be in a sandbox here.  Kinda hard to tell what state things are in in this testcase.  ;)

Comment 12

[georgi - hopefully not receiving bugspam]

well, the testcase is quite perverted :)

the sandbox is found via binary search with binaries?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes, though I'd start with testing builds before and after bug 332182 landed.

Comment 14

[georgi - hopefully not receiving bugspam]

are there debug builds somewhere ?

Comment 15

[Boris Zbarsky [:bzbarsky]]

No... but you don't need a debug build to see whether the error happens, right?

Comment 16

[georgi - hopefully not receiving bugspam]

well, it is not quite needed indeed, bug debug builds have |window.dump| and |debugger| which may automate the binary search by a program if the testcase dumps to stdout |pass| or |fail|.

20060812 - 20060817 doesn't seem to introduce the sandbox stuff.
the first one works fine, the second one gives security error.

Comment 17

[Boris Zbarsky [:bzbarsky]]

window.dump() works in opt builds if you set the relevant pref... something something dump_enabled, iirc.

Comment 18

[georgi - hopefully not receiving bugspam]

thanks.

browser.dom.window.dump.enabled = true

Comment 19

[georgi - hopefully not receiving bugspam]

if i have done the search right |window is not defined| appeared first in
2006-10-17-04-trunk
2006-10-16-04 doesn't give error

Comment 20

[georgi - hopefully not receiving bugspam]

bclary, have you tested this:
http://www.st.cs.uni-sb.de/dd/
Delta Debugging
From automated testing to automated debugging

http://www.infosun.fim.uni-passau.de/st/papers/tr-99-01/
Our delta debugging prototype tracked down a single failure-inducing change from 178,000 changed GDB lines within a few hours.

Comment 21

[Brendan Eich [:brendan]]

See also http://delta.tigris.org/.

/be

Comment 22

[georgi - hopefully not receiving bugspam]

btw, binary search is not 100% correct if there are 2 or more regressions - it catches *only one* of them?

Comment 23

[georgi - hopefully not receiving bugspam]

i mean the state is

GOOD REGRESSION1 BAD FIX1 GOOD REGRESSION2 BAD

binary search catches only REGRESSION1 or REGRESSION2 but not both of them

Comment 24

[georgi - hopefully not receiving bugspam]

offtopic:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsDOMClassInfo.cpp&rev=1.448&mark=6512-6522#6512
6512                     nsNodeSH::SetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
 6513                                           JSObject *obj, jsval id, jsval *vp, PRBool *_retval)
 6514                     {
 6515                       if ((id == sBaseURIObject_id || id == sNodePrincipal_id) &&
 6516                           IsPrivilegedScript()) {
 6517                         // Is there a better error we could use here?  We don't want privileged
 6518                         // script that can read these properties to set them, but _do_ want to
 6519                         // allow everyone else to.
 6520                         return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
 6521 bzbarsky      1.394   }
 6522                     

 6523                       return nsEventReceiverSH::SetProperty(wrapper, cx, obj, id, vp,_retval);
 6524                     }

so everything except privileged script may set nodePrincipal - the comment claims this is on purpose.

chrome seems to do checks with doc.nodePrincipal

Comment 25

[Bob Clary [:bc] (inactive)]

(In reply to comment #20)
> bclary, have you tested this:
>

No, but I'm reading now.

(In reply to comment #21)
> See also http://delta.tigris.org/.

That looks like something I could adapt pretty easily.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> offtopic:

For what it's worth, that would be better in a bug of its own or private mail not in unrelated bugs.  I'll answer this once, but in general I'm going to ignore comments like that in a spam-reduction attempt.  ;)

We could use a secure mailing list for this sort of thing....

> so everything except privileged script may set nodePrincipal

Yes.  On its JS object, which is not the same as what privileged script sees.

> chrome seems to do checks with doc.nodePrincipal

Different nodePrincipal, thanks to XPCNativeWrapper.

Back to this bug, the regression range in comment 19 is http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-10-16+04&maxdate=2006-10-17+04&cvsroot=%2Fcvsroot

which corresponds to bug 351633.  I suspect the new behavior is what we want, but we'll need to sort through the testcase to make sure.  I have no idea when I'll have time to do that, though. :(

Comment 27

[georgi - hopefully not receiving bugspam]

(In reply to comment #26)
> > offtopic:
> 
> For what it's worth, that would be better in a bug of its own or private mail
> not in unrelated bugs.  I'll answer this once, but in general I'm going to

ok, sorry.

thought that a new bug or private mail will generate more spam from your point of view.

Comment 28

[Damon Sicore (:damons)]

I'm not seeing the error initially reported, using the attachment.  Is this bug still valid?

Comment 29

[Bob Clary [:bc] (inactive)]

I see Error: uncaught exception: ReferenceError: window is not defined in today's Minefield on Linux.

Comment 30

[Johnny Stenback (:jst)]

I looked into this a bit and the reason we end up evaluating things in a sandbox is that on reload of the testcase after clicking the link we're loading a javascript: URI whose channel owner is bugzilla (the testcase) but the object principal of the window in which it's loading is moz-safe-about:blank.

Boris, does that make sense to you?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Hmm.  How are we ending up with an about:blank principal here?  I wouldn't expect that to happen...

Comment 32

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  A history load of a javascript: URI does CreateAboutBlankContentViewer with a null principal (to make sure that the JS doesn't run against whatever is loaded in the browser right now).

I think it would make sense to give that particular about:blank the principal of the javascript: load, no?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Not security-sensitive.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, say

Comment 35

[Johnny Stenback (:jst)]

Comment on attachment 283813 [details] [diff] [review]
Like so, say

Yeah, that would be the right thing to do.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Checked in.  Need to write a testcase.... presumably using nodePrincipal on an iframe or something?  If someone is willing to do that, that would be much appreciated.  Just loading a javascript: URI and then reloading it should work.

Comment 37

[georgi - hopefully not receiving bugspam]

Attachment: testcase for the fix

testcase for the fix
doesn't seem to pass

Comment 38

[georgi - hopefully not receiving bugspam]

Attachment: testcase2 - with setTimeout, passes on today trunk, fails on earlier and branch

Comment 39

[georgi - hopefully not receiving bugspam]

hm, selecting |details| on testcase2 with branch build leads to busy cursor probably due to nested iframes

Comment 40

[Boris Zbarsky [:bzbarsky]]

Should probably use a load listener to wait for the reload instead of using a race-prone timeout.

Comment 41

[georgi - hopefully not receiving bugspam]

Attachment: testcase3 using onload

Comment 42

[Andrew Schultz]

I was trying a testcase as described in comment 36, but (before the patch) the iframe reload fails and the load event doesn't fire.

Comment 43

[georgi - hopefully not receiving bugspam]

bclary, how we do put "testcase3" in testsuite?
probably the only change is replacing |alert| with the automatic check function.

Comment 44

[Boris Zbarsky [:bzbarsky]]

This is not in testsuite yet.  And the JS testsuite is not the right place for it, since it has no idea about about:blank or security.

I'll work on automating this when I have time...

Comment 45

[Bob Clary [:bc] (inactive)]

I agree with bz. It "could" be shoe horned into the js test suite, but it has a better future in the mochikit related tests.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Added mochitest.