"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

Why do you think this should be security sensitive?

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.

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

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...

(In reply to comment #4) > Is every single piece of the testcase needed? > not quite sure, but have troubles minimizing it more...

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?

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.

(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?

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

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

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. ;)

well, the testcase is quite perverted :) the sandbox is found via binary search with binaries?

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

are there debug builds somewhere ?

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

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.

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

thanks. browser.dom.window.dump.enabled = true

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

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.

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

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

i mean the state is GOOD REGRESSION1 BAD FIX1 GOOD REGRESSION2 BAD binary search catches only REGRESSION1 or REGRESSION2 but not both of them

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

(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.

> 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. :(

(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.

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

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

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?

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

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?

Not security-sensitive.

Comment on attachment 283813 [details] [diff] [review] Like so, say Yeah, that would be the right thing to do.

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.

testcase for the fix doesn't seem to pass

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

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

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.

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

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...

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.

Added mochitest.