768355 - Web console has a chrome entry script base URI; should be using the page instead

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[mauro.bieg]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.56 Safari/536.5

Steps to reproduce:

1.) go to http://www.tinymce.com/tryit/full.php
2.) Enter the following in the web console: document.getElementById('content_ifr').contentWindow.location = "#bug"

Firefox 13.0.1 on Mac OS X 10.7.4


Actual results:

A new Firefox window including chrome is rendered inside the iframe


Expected results:

The iframe should scroll down to where an object with id "bug" is (if any exists), otherwise do nothing.

similarily, entering:
document.getElementById('content_ifr').contentWindow.location.hash = "#bug"
renders also the contents of the parent frame inside the iframe, this time without browser chrome though.

Comment 1

[Boris Zbarsky [:bzbarsky]]

When you set location to a relative URI from a script, the relative URI is resolved with respect to the base URI of the _script_, not of the window the location object is on.

So the real bug here sounds like the base URI of scripts in the web console is wrong...

As far as setting .hash, that uses the existing URI of the window.  The behavior you see there is covered by bug 302976.

Back to the web console thing... the way nsLocation gets the base URI is by using nsJSUtils::GetDynamicScriptGlobal on the top of the JSContext stack and then using the base URI of its document.  Which means that in this case we just need to make sure the web console is evaluating its stuff on a JSContext whose script global is the content window, not the chrome window.  Or more precisely that such a JSContext gets pushed on the stack (possibly in the sandbox code?).  Bobby, would it make sense of evalInSandbox to always push the sandbox context on the stack?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> Or more precisely that such a JSContext gets pushed on the stack (possibly
> in the sandbox code?).  Bobby, would it make sense of evalInSandbox to
> always push the sandbox context on the stack?

I think so, yeah - at least until we have the entry script stack.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  So actually, sandbox code _does_ push the sandbox context.  This has the ContextHolder as private and the sandbox as the global.

The problem is that nsLocation::SetHref calls GetContextFromStack which doesn't just use the top context on the stack: it walks the stack looking for a JSContext for which GetScriptContextFromJSContext returns non-null.  Which it doesn't for the sandbox context, because SandboxPrivate doesn't QI to nsIScriptContext.

What I see over here is that it never finds such a context, and then loads the page URL in the frame, not the "browser UI" bit reporter is seeing.

But in any case, the point is that GetDynamicScriptGlobal/GetDynamicScriptContext on a sandbox won't return the relevant Window or its context.  That seems wrong to me...  I can see the latter, but the former seems like we should fix it.

Comment 4

[Bobby Holley (:bholley)]

Oh yeah, I actually fixed that in one of the patches over in bug 758344.

I wasn't planning to land those patches on m-c, since the second one conflicts with bug 754202, which removes all that machinery. But I could probably land the other one to m-c since it's unclear how long it will take to remove the GetDynamicScriptContext stuff.

bz, does that patch fix this bug? If so, do you think I should land it to m-c?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well, for now I can't reproduce the bug, as filed, so hard to test things.  I suppose I could step through SetHref...

Comment 6

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Well, for now I can't reproduce the bug, as filed, so hard to test things. 
> I suppose I could step through SetHref...

I can also just land the patch to inbound if you think it's a good idea. It has review.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I think that would be a good idea, yeah.

Comment 8

[Bobby Holley (:bholley)]

pushed.

Comment 9

[mauro.bieg]

Great to see that something is being done! Just wanted to add that it doesn't happen in the Web Console only. I have a TinyMCE config where on a button's press the mentioned line of JavaScript is called and the same thing happens as in the Web Console.

Comment 10

[Boris Zbarsky [:bzbarsky]]

"Same thing" as in the browser UI getting loaded in the iframe?

If by "same thing" you mean the toplevel page being loaded in the iframe, that's bug 302976.

Comment 11

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Location uses GetEntryDocument() now, and there is only one JSContext around.  So this is not a problem anymore.