307983 - NS_ERROR_INVALID_POINTER when using location.* inside evalInSandbox

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P2)

Description

[Aaron Boodman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

When you access what appears to be any property of the location object (I am
interested mainly in .href and .replace) from within evalInSandbox, you get an
NS_ERROR_INVALID_POINTER error.

For instance, with this code:

var box = new Components.utils.Sandbox(location.href);
box.window = window;
Components.utils.evalInSandbox("alert(window.location.href)", box);

Reproducible: Always

Steps to Reproduce:
1. Open the attachment in FF
2. Say yes to the security prompt
3. Note error in javascript console

Actual Results:  
Error in JS Console:

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMLocation.replace]"  nsresult:
"0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame ::
file:///C:/Documents%20and%20Settings/aa/Desktop/evalInSandbox.html ::
<TOP_LEVEL> :: line 8"  data: no]

Expected Results:  
alert box with URL of current window

I noticed that for some reason you don't get the error if you wrap the code that
access the location object in a window.setTimeout call.

Not sure if that is helpful to you at all. So for instance, this *does not* have
the error:

Components.utils.evalInSandbox("window.setTimeout(function(){alert(window.location.href)})",
box);

Comment 1

[Aaron Boodman]

Attachment: testcase

Bollocks, forgot attachment again.

Comment 2

[Ian Macfarlane]

Should this block 1.5 final as it is stopping GreaseMonkey from working properly?

See: http://greaseblog.blogspot.com/2005/09/firefox-15-compatible-greasemonkey.html

Comment 3

[Asa Dotzler [:asa]]

sounds like a major regression that impacts a popular extension. Pulling into
the blocking radar, but time is really short and we don't yet have anyone signed
up to investigate and fix. Aaron, if you can help us find resources, or figure
out exactly what broke this, that'd be a big help.

Comment 4

[Phil Ringnalda (:philor)]

Bad timing: Aaron's in Vietnam until the end of this week, I think.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I'll try to pull this in. I'm hoping there's going to be a simple fix for this.
I really wish that I'd known about this weeks ago :-(.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I'm hoping that this fix will be evalInSandbox-specific (and thus low-risk).

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Something like this

This, sadly, isn't evalInSandbox-specific. It gets the current window off of
the docshell if it can't get it off of the context. I think this can only
happen in the "extensions" case. jst, what do you think?

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 199235 [details] [diff] [review]
Something like this

r- based on discussion with mrbkap. This would make us use the wrong URL when
as the base when resolving relative URLs on location objects from a window
other than the callers.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated to comments

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 199607 [details] [diff] [review]
updated to comments

*sigh*, the iterator needs to go _backwards_ (iterating over a stack, and all).

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: with those changes

This has r=jst looking over my shoulder.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 199619 [details] [diff] [review]
with those changes

I nagged mrbkap on IRC about not sharing the common return from
GetContextFromStack, allowing the break to become a return NS_OK and the final
return to be immediately preceded by an unconditional *aContext = nsnull.

/be

Comment 13

[Asa Dotzler [:asa]]

Blake, we need this landed and verified on the trunk ASAP. Thanks.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the trunk (with another small tweak to make the iterator handle
an empty deque).

Comment 15

[[e-mail]]

Neither of my depends builds register the new iterator, which busts all sorts of
things :-( BTW, why is ContextStackIteror not spelled correctly?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

FYI, I fixed the misspelling (thanks copy/paste) on the trunk today. Neil, I'm
not sure why your builds are failing. I can't reproduce that problem either on
my Linux build or my Windows build...

Comment 17

[[e-mail]]

Seems to have been some weird compreg.dat issue, it didn't reregister properly.
Deleting compreg.dat fixed that and also found some obsolete components ;-)

Comment 18

[Asa Dotzler [:asa]]

Neil, is that a verification? Have you got a working build yet?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Checked into MOZILLA_1_8_BRANCH.