new Option() fails in sandbox (sandbox never pushes its JSContext* into the JS context stack)

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XPConnect
P3
major
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Matthias Bauer, Assigned: mrbkap)

Tracking

({fixed1.8, regression})

Trunk
mozilla1.9alpha1
x86
Windows XP
fixed1.8, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: needed for bug 314865)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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
true
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.

Updated

12 years ago
Component: General → JavaScript Console
Keywords: regression
Product: Firefox → Core
Version: unspecified → Trunk

Updated

12 years ago
Assignee: nobody → general
Component: JavaScript Console → JavaScript Engine
QA Contact: general → general
(Reporter)

Comment 1

12 years ago
It seems that window.__proto__.Option is not defined within the sandbox.
(Reporter)

Comment 2

12 years ago
Changed summary
Summary: new Option() fails in sandbox, exception says 'null' (Greasemonkey) → Option prototype undefined in sandbox (new Option() fails)

Comment 3

12 years ago
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.
http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpccomponents.cpp#2403

-----
|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.
Assignee: general → dbradley
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → XPConnect
Ever confirmed: true
QA Contact: general → pschwartau
Summary: Option prototype undefined in sandbox (new Option() fails) → new Option() fails in sandbox (sandbox never pushes its JSContext* into the JS context stack)
Thanks, shutdown.  I think we should get a fix together ASAP for 1.8/fx1.5 -- this is a GreaseMonkey platform regression.

/be
Assignee: dbradley → mrbkap
Flags: blocking1.8rc2?
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?

/be
(Assignee)

Comment 6

12 years ago
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).
Attachment #201649 - Flags: superreview?(shaver)
Attachment #201649 - Flags: review?(brendan)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.8rc1
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.

/be
Attachment #201649 - Flags: superreview?(shaver)
Attachment #201649 - Flags: review?(brendan)
(Assignee)

Comment 8

12 years ago
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.
Target Milestone: mozilla1.8rc1 → mozilla1.9alpha
(Assignee)

Comment 9

12 years ago
Created attachment 201663 [details] [diff] [review]
Updated to comments
Attachment #201649 - Attachment is obsolete: true
Attachment #201663 - Flags: review?(brendan)
Comment on attachment 201663 [details] [diff] [review]
Updated to comments

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

/be
Attachment #201663 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

12 years ago
Created attachment 201676 [details] [diff] [review]
fixing my oops
Attachment #201663 - Attachment is obsolete: true
Attachment #201676 - Flags: superreview?(shaver)
Attachment #201676 - Flags: review+
Comment on attachment 201676 [details] [diff] [review]
fixing my oops

sr=shaver. who wrong this garbage anyway?
Attachment #201676 - Flags: superreview?(shaver) → superreview+
(Assignee)

Comment 13

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

12 years ago
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.
Attachment #201676 - Flags: approval1.8rc2?

Comment 15

12 years ago
(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.

Updated

12 years ago
Attachment #201676 - Flags: approval1.8rc2? → approval1.8rc2+
(Assignee)

Comment 16

12 years ago
Checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
Blocks: 314865

Comment 17

12 years ago
flag cleanup for a bug fixed on the branch already.
Flags: blocking1.8rc2?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: needed for bug 314965
We don't think we need this for the older branches, see bug 314865
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: needed for bug 314965 → needed for bug 314865
You need to log in before you can comment on or make changes to this bug.