Closed Bug 313220 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XPConnect, defect, P3)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: bugzilla, Assigned: mrbkap)

References

Details

(Keywords: fixed1.8, regression, Whiteboard: needed for bug 314865)

Attachments

(1 file, 2 obsolete files)

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.
Component: General → JavaScript Console
Keywords: regression
Product: Firefox → Core
Version: unspecified → Trunk
Assignee: nobody → general
Component: JavaScript Console → JavaScript Engine
QA Contact: general → general
It seems that window.__proto__.Option is not defined within the sandbox.
Changed summary
Summary: new Option() fails in sandbox, exception says 'null' (Greasemonkey) → Option prototype undefined in sandbox (new Option() fails)
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
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)
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)
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
Attached patch Updated to comments (obsolete) — Splinter Review
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+
Attached patch fixing my oopsSplinter Review
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+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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?
(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.
Attachment #201676 - Flags: approval1.8rc2? → approval1.8rc2+
Checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
Blocks: 314865
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.