Last Comment Bug 313220 - new Option() fails in sandbox (sandbox never pushes its JSContext* into the JS context stack)
: new Option() fails in sandbox (sandbox never pushes its JSContext* into the J...
needed for bug 314865
: fixed1.8, regression
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Windows XP
: P3 major (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
: Phil Schwartau
: Andrew Overholt [:overholt]
Depends on:
Blocks: 314865
  Show dependency treegraph
Reported: 2005-10-20 18:22 PDT by Matthias Bauer
Modified: 2006-02-10 15:46 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix the problem pointed out by shutdown (2.06 KB, patch)
2005-11-02 11:08 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Updated to comments (2.27 KB, patch)
2005-11-02 12:23 PST, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review
fixing my oops (2.31 KB, patch)
2005-11-02 13:56 PST, Blake Kaplan (:mrbkap)
mrbkap: review+
shaver: superreview+
asa: approval1.8rc2+
Details | Diff | Splinter Review

Description Matthias Bauer 2005-10-20 18:22:20 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

Trying to instatiate an Option object from within a Greasemonkey user script
under Firefox 1.5 silently fails. Catching the exception yields 'null'. The same
happens with other forms of the Option constructor (e.g. new Option('title'), etc.).

Greasemonkey injects scripts by using a sandbox (Components.utils.Sandbox in FF
beta, Components.utils.evalInSandbox in DP alpha). Creating a new Option()
anywhere else (in the location bar, Venkman, html pages, etc.) all work.

Reproducible: Always

Steps to Reproduce:
1. Create a new Greasemonkey user script
2. Put: try { var x = new Option(); alert(x); } catch(e) { GM_log('exception
creating new Option: '+e); }
3. Save user script and set includes
4. in about:config, create a boolean pref "greasemonkey.logChrome" and set it to
5. Visit a page that triggers the script (e.g. an included page)

Actual Results:  
Greasemonkey log message in the JS console: 'exception creating new Option: null'

Expected Results:  
Alert window saying [HTMLOptionElement]

Problem started occuring with Firefox 1.5 beta 1.
Works using Firefox 1.0.7 and below.
Comment 1 Matthias Bauer 2005-10-21 15:50:21 PDT
It seems that window.__proto__.Option is not defined within the sandbox.
Comment 2 Matthias Bauer 2005-10-26 17:36:54 PDT
Changed summary
Comment 3 shutdown 2005-11-02 06:35:16 PST
The value of |window.__proto__.Option| isn't a problem here.
The |Option| constructor is accessable via the scope chain.

The problem here is nsXPCComponents_Utils::EvalInSandbox()
never pushes the sandbox's JSContext* into the JS context stack.

|new Option(args)| in JS invokes the native function CreateHTMLOptionElement().
CreateHTMLOptionElement() then invokes the function NS_NewHTMLOptionElement()
with a null nodeinfo. If NS_NewHTMLOptionElement() is invoked with a null
nodeinfo, it retrieves the document for the newly created OPTION element
from the JS stack using nsContentUtils::GetDocumentFromCaller(),

However, nsContentUtils::GetDocumentFromCaller() can't find the correct
document and returns null if |new Option(args)| is used inside the sandbox.
This is because nsIThreadJSContextStack#Peek(), which is used by
GetDocumentFromCaller(), is impossible to retrieve the correct JSContext*.

CONFIRMING. Updating the summary and moving to the XPConnect component.
Comment 4 Brendan Eich [:brendan] 2005-11-02 09:43:11 PST
Thanks, shutdown.  I think we should get a fix together ASAP for 1.8/fx1.5 -- this is a GreaseMonkey platform regression.

Comment 5 Brendan Eich [:brendan] 2005-11-02 10:14:58 PST
Of course, the full fix to bug 298064 (see also bug 312363) would fix this bug too.  So we could fix this particular bug by fixing 298064 (although the patch looks likely to appear in 312363).

I'm not saying this is a dup, however, or even a dependent bug.  The question is, are there other reasons to push the sandcx on the thread context stack?

Comment 6 Blake Kaplan (:mrbkap) 2005-11-02 11:08:58 PST
Created attachment 201649 [details] [diff] [review]
Fix the problem pointed out by shutdown

Yeah, this seems to work (note: I didn't install greasemonkey, and instead tried to use a simplified testcase).
Comment 7 Brendan Eich [:brendan] 2005-11-02 12:14:27 PST
Comment on attachment 201649 [details] [diff] [review]
Fix the problem pointed out by shutdown

>+    XPCPerThreadData *data = XPCPerThreadData::GetData();
>+    XPCJSContextStack *stack;
>+    PRBool popContext = PR_FALSE;
>+    if (data && (stack = data->GetJSContextStack()) &&

So far so good, as you said in person, to soldier on if there's no XPConnect per-thread data, or (this may be an error) there is such data, but a null stack.

>+        NS_SUCCEEDED(stack->Push(sandcx))) {

This error should fail the whole evalInSandbox.

Fix that and r=me.

Comment 8 Blake Kaplan (:mrbkap) 2005-11-02 12:22:38 PST
Note that there is a very simply workaround to use in GreaseMonkey: to use document.createElement("option") instead of |new Option()|. That makes me doubt that this would be a 1.8 showstopper.
Comment 9 Blake Kaplan (:mrbkap) 2005-11-02 12:23:24 PST
Created attachment 201663 [details] [diff] [review]
Updated to comments
Comment 10 Brendan Eich [:brendan] 2005-11-02 12:59:59 PST
Comment on attachment 201663 [details] [diff] [review]
Updated to comments

Fix to destroy-no-GC the sandcx before the failure return and r=me.

Comment 11 Blake Kaplan (:mrbkap) 2005-11-02 13:56:03 PST
Created attachment 201676 [details] [diff] [review]
fixing my oops
Comment 12 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-11-02 13:59:12 PST
Comment on attachment 201676 [details] [diff] [review]
fixing my oops

sr=shaver. who wrong this garbage anyway?
Comment 13 Blake Kaplan (:mrbkap) 2005-11-02 14:11:06 PST
Fix checked into trunk.
Comment 14 Blake Kaplan (:mrbkap) 2005-11-02 14:16:42 PST
Comment on attachment 201676 [details] [diff] [review]
fixing my oops

Requesting 1.8rc2 approval. This fix is believed to be very safe (it uses existing code to make evalInSandbox behave like a good XPConnect function, pushing the current JS context onto the context stack). The fix is localized to evalInSandbox, so if it does break anything, it will be GreaseMonkey specific. I've tested PAC with this patch, and it still works.
Comment 15 shutdown 2005-11-02 18:47:21 PST
(In reply to comment #5)
> I'm not saying this is a dup, however, or even a dependent bug.  The question
> is, are there other reasons to push the sandcx on the thread context stack?
> /be

Breakage of the sandbox's security model. See bug 314865.
Comment 16 Blake Kaplan (:mrbkap) 2005-11-02 19:15:45 PST
Checked into MOZILLA_1_8_BRANCH.
Comment 17 Scott MacGregor 2005-11-03 15:02:54 PST
flag cleanup for a bug fixed on the branch already.
Comment 18 Daniel Veditz [:dveditz] 2006-02-10 15:46:29 PST
We don't think we need this for the older branches, see bug 314865

Note You need to log in before you can comment on or make changes to this bug.