Right now nsContentUtils::GetDocumentFromCaller() is broken, it now gets the document from the calling context, which may or may not be what you want, what you most likely always want is the document where the calling code came from. We use the caller document in document.open() etc to propagate security info etc, and that really should be that of the calling code, not the calling context.

Luckily it's not used much http://lxr.mozilla.org/mozilla/search?string=GetDocumentFromCaller

Blocking beta, but not branch.

Peter - can you take a look?

This seems to do the trick (thanks to jst for setting me on the right track). Brendan: are we doing the right thing here by using JS_GetScopeChain/JS_GetParent, it does work but is it correct? Jst: shouldn't the code to get callerPrincipal be moved out of the if at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855 ?

(In reply to comment #4) > Created an attachment (id=193916) [edit] > v1 > > This seems to do the trick (thanks to jst for setting me on the right track). > Brendan: are we doing the right thing here by using > JS_GetScopeChain/JS_GetParent, it does work but is it correct? > Jst: shouldn't the code to get callerPrincipal be moved out of the if at > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855 > ? Yeah, seems like it should be. Wanna do a double whammy and include that in this fix as well? :)

(In reply to comment #5) > (In reply to comment #4) > > Jst: shouldn't the code to get callerPrincipal be moved out of the if at > > > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855 > > ? > > Yeah, seems like it should be. Wanna do a double whammy and include that in this > fix as well? :) Sure, I'll try to attach a new patch with that included later tonight.

Comment on attachment 193935 [details] [diff] [review] v1.1 Ok, you don't want the scope chain, if we are willing to break compatibility with Gecko here in the interest of sanity and (perhaps, need to test; also likely irrelevant) Netscape 2-4. You want to change GetDocumentFromCaller to take a JSObject *obj argument that it uses as the start of the loop up the parent chain to a global object. This obj param should be the Image constructor, e.g., being called. That is, var img = new Image will work as with your patch, but var img2 = new otherWindow.Image will work as it should, not as it does today (which I believe involves using the wrong document's nodeinfo list -- please correct me if I am wrong). Need a third testcase covering the img2 case and somehow detects the difference in results, and some results from various browsers. /be

(In reply to comment #10) > (From update of attachment 193935 [details] [diff] [review] [edit]) > Ok, you don't want the scope chain, if we are willing to break compatibility > with Gecko here in the interest of sanity and (perhaps, need to test; also > likely irrelevant) Netscape 2-4. Of course, your patch to use the scope chain is incompatible with the broken, longstanding Gecko implementation anyway. Still, it'd be good to have a testcase and results from various browsers. We may choose to break compat here because no one will care. I'm keen to see how you'd detect the difference in which document's nodeinfo list was used. /be

So I either need to walk the JS stack until I find a function or a constructor and use the function object or the constructor object, or I need to pass those objects on from where I can reach them. The first solution works if I add a JS_GetFrameConstructorObject to jsdbgapi.h/cpp which does |return fp->argv && (fp->flags & JSFRAME_CONSTRUCTING) != 0 ? JSVAL_TO_OBJECT(fp->argv[-2]) : NULL;|. The other solution works too, I can get at argv[-2] from nsHTMLImageElement::Initialize and nsHTMLOptionElement::Initialize, but it means that we have to create elements without a nodeinfo (Initialize gets called after creation). I'd rather not do that (and avoid the "No nsINodeInfo passed to nsGenericElement, PREPARE TO CRASH!!!" assertion), so I'm leaning towards the first solution. I need to know whether adding JS_GetFrameConstructorObject would be ok though.

(In reply to comment #12) > I need to know > whether adding JS_GetFrameConstructorObject would be ok though. I don't see the need, given JS_GetFrameFunctionObject -- why not use just that, or condition it with JS_IsConstructing? /be

Really, it doesn't matter whether the JS callee is a constructor or not. If you are hacking a native method reached from a new Image or Image() call or whatever, and it's creating a new Image object, you want it to use static scoping. /be

(In reply to comment #13) > I don't see the need, given JS_GetFrameFunctionObject -- why not use just that, > or condition it with JS_IsConstructing? Because JS_GetFrameFunctionObject checks |fp->argv && fp->fun| and when coming through a constructor (like from new Image()) fp->fun is null.

IE 6, Opera 8 and Safari 2.0 all pass this one, and with my patch (as described in comment 12) Firefox does too. It's not trivial to test this in older browsers, I guess it could be done but do we really care given these results?

(In reply to comment #15) > (In reply to comment #13) > > I don't see the need, given JS_GetFrameFunctionObject -- why not use just that, > > or condition it with JS_IsConstructing? > > Because JS_GetFrameFunctionObject checks |fp->argv && fp->fun| and when coming > through a constructor (like from new Image()) fp->fun is null. Oh duh -- XPConnect makes its own callable objects. Ok, all we really need here is fp->argv[-2], with safety null-checking for fp->argv. Don't conflate it with JS_IsConstructing, call it JS_GetFrameCalleeObject and add it to the patch? /be

Comment on attachment 194235 [details] [diff] [review] v2 Thanks, looks good, only thought: Isn't there a subroutine to get the global object by walking up the parent chain? Maybe jst knows. /be

(In reply to comment #21) > Isn't there a subroutine to get the global object by walking up the parent > chain? Maybe jst knows. There's nsJSUtils::GetStaticScriptGlobal, it would add one QI (it returns nsIScriptGlobalObject and we want nsPIDOMWindow). Jst?

Comment on attachment 194235 [details] [diff] [review] v2 Yeah, use nsJSUtils here, one more QI won't hurt much. r=jst

Here's the patch that I checked in on the trunk. Carrying over r=jst, sr=brendan and asking for branch approval.

I'll verify this with tomorrow's builds.

(In reply to comment #18) > Created an attachment (id=194172) [edit] > Testcase file A > Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050903 Firefox/1.6a1 I'll try to verify on Linux and Mac later.

This appears to have caused bug 312363, showing mixed content if a child frame has been document.written() into as seen in a real-world bank site.

The nsContentUtils.cpp changes that went in with this fix was backed out on the branch. See bug 312363. (removing fixed1.8 keyword)

Should this bug be reopened, and used for patching to fix the two callers (for new Image and new Option from JS) that need fixing? Bug 312363 is about document.open and it is fixed. /be

Brendan, this patch is still in on the trunk. I had the intention of leaving things that way and simply fixing document.open on the trunk (I'm about to attach a new patch to that bug).

fyi, failed ff 1.0.8/winxp/20060221

This is not fixed in bon echo 1.8 20060910/winxp.

(In reply to comment #32) > This is not fixed in bon echo 1.8 20060910/winxp. Yeah, the patch was backed out from the 1.8 branch (see comment 28). If we want this fixed on the trunk, the trunk patch in bug 312363 should be nominated/checked into the 1.8 branch.

Bkap/Brendan - what do we need to do here?

(In reply to comment #33) > (In reply to comment #32) > > This is not fixed in bon echo 1.8 20060910/winxp. > > Yeah, the patch was backed out from the 1.8 branch (see comment 28). If we want > this fixed on the trunk, I think Blake meant "branch" here. > the trunk patch in bug 312363 should be > nominated/checked into the 1.8 branch. That sounds like the thing to do for 1.8. Blake, can you confirm? /be

(In reply to comment #35) > That sounds like the thing to do for 1.8. Blake, can you confirm? Yeah, that's what I meant.

Could you nominate the appropriate patch? It's marked fixed1.8.

(In reply to comment #37) > Could you nominate the appropriate patch? It's marked fixed1.8. It's marked fixed1.8 because we backed out the offending patch from the branch. I'll nominate the correct patch now (checking to make sure that it applies correctly first).

Plusing the blocking flag on this so we don't lose it. Sounds like we want the patch in 312363...

Moving this out to 1.8.1.1 since we can't seem to get a straight answer on what the right approach here is...

mrbkap: sounds like we want this one, or some kind of fix, but what patch? Please ask for approval on what we need. Would we want this for 1.8.0.9 too?

Removing blocking flags, peterv wants us to take the fix in bug 312363. I will update that bug with the flags.

Fixed on branches by the check-in of attachment 238242 [details] [diff] [review] from bug 312363.

Verified no crash using testcase on comment #18 with the following: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061128 Minefield/3.0a1 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre and 1.8.0.9 on Intelmac, 1.8.1.1 linux

I checked in a test for this in bug 445004.