[FIX]Sort out XPCNativeWrapper behavior for calls from C++ to JS

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XPConnect
P1
major
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug, {fixed1.8.0.5, fixed1.8.1})

Trunk
mozilla1.9alpha1
x86
Linux
fixed1.8.0.5, fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [need testcase])

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

11 years ago
See comments in bug 336875.  Basic issue is that when calling from C++ to JS we currently use whatever random script is on the stack to determine whether to create an XPCNativeWrapper.  This is wrong.  I propose to condition the current behavior on ccx.GetXPCContext()->CallerTypeIsJavaScript().

I also think I can get at the script filename of the thing we're calling into, with a bit of work, so things that set xpcnativewrappers=no will continue to work right.

The question is whether this is a behavior change we're willing to take on the stable branch.  I think we should, but it's kinda scary...  To avoid breaking adblock we should also make sure that Components.utils.lookupMethod works with XPCNativeWrappers; I don't think it does right now.  Can anyone think of other problems this will cause?
(Assignee)

Comment 1

11 years ago
Created attachment 221376 [details] [diff] [review]
Like so
Attachment #221376 - Flags: superreview?(brendan)
Attachment #221376 - Flags: review?(jst)
Comment on attachment 221376 [details] [diff] [review]
Like so

I'm going to be scarce for a while here and at XTech next week, but:

* factor jsdbgapi.h into minimal pieces: that means just JS_GetScriptFilenameFlags, and the caller has to go from function to script using JS_GetFunctionScript (and JS_GetPrivate if necessary).

* Nits: JS does not use the C++ wrong-headed style of cuddling * with the typename on its left -- * is a declarator mode, it cuddles the declarator name.

/be
(Assignee)

Comment 3

11 years ago
> that means just JS_GetScriptFilenameFlags

So the loop over cx->fp should also move into the caller?

I'll make the other changes.
(In reply to comment #3)
> > that means just JS_GetScriptFilenameFlags
> 
> So the loop over cx->fp should also move into the caller?

I perpetrated JS_GetTopScriptFilenameFlags in an attempt to make an ad-hoc API, instead of factoring fully.  I regret this error :-/.  Is it undo-able?  I think so, especially if we can get the factoring fix into the js1.6 standalone release.

> I'll make the other changes.

Thanks,

/be
(Assignee)

Comment 5

11 years ago
Created attachment 221761 [details] [diff] [review]
With Brendan's issues addressed
Assignee: dbradley → bzbarsky
Status: NEW → ASSIGNED
Attachment #221761 - Flags: superreview?(brendan)
Attachment #221761 - Flags: review?(jst)
(Assignee)

Comment 6

11 years ago
We should also figure out whether we want this for the 1.8 branches.  It's sorta an API change, a bit....
Priority: -- → P1
Summary: Sort out XPCNativeWrapper behavior for calls from C++ to JS → [FIX]Sort out XPCNativeWrapper behavior for calls from C++ to JS
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

11 years ago
Attachment #221376 - Attachment is obsolete: true
Attachment #221376 - Flags: superreview?(brendan)
Attachment #221376 - Flags: review?(jst)
(Assignee)

Updated

11 years ago
Flags: blocking1.9a1?
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?

Comment 7

11 years ago
Can't take this for 1.8.0.5 due to API change risks.  If there is a safer fix, we can reconsider.
Flags: blocking1.8.0.5? → blocking1.8.0.5-
(Assignee)

Comment 8

11 years ago
Jay, the "API change" is that stuff that used to be unsafe is now made safe.  This does change what chrome code sees in some cases, of course -- it no longer sees unsafe stuff.

There's no way to make this "safer" short of not taking the security fix.

Renominating; I'd like us to do an actual evaluation of the security issue and then deciding how much we care about fixing it.
Flags: blocking1.8.0.5- → blocking1.8.0.5?
Marking blocking for 1.8.0.5, but with jst and brendan out on vacation right now it'll be tight to get it into the release. We'll probably have to forego our usual requirement to get trunk baking time, so we'll need extra QA attention to this on the branches.
Flags: blocking1.8.0.5? → blocking1.8.0.5+
(Assignee)

Comment 10

11 years ago
I bet dbradley, shaver, mrbkap would be qualified to review this (though I'd still like jst and brendan to OK the concept and brendan in particular to OK the jsdbgapi.h change...

Not changing the review flags because I'm not sure who all is free, exactly, but I'd love drive-by reviews.
(Assignee)

Updated

11 years ago
Attachment #221761 - Flags: review?(mrbkap)
I don't think this is the patch we want on the 1.8.0 branch, where we should leave the old API in place and just add the new one for this new caller.  (We're already breaking bincompat in 1.8.1, so we can follow the trunk there.)
Comment on attachment 221761 [details] [diff] [review]
With Brendan's issues addressed

r=jst, but I agree with shaver on leaving the old API in place on the branch and just adding the new one.
Attachment #221761 - Flags: review?(jst) → review+
(Assignee)

Comment 13

11 years ago
Comment on attachment 221761 [details] [diff] [review]
With Brendan's issues addressed

shaver says sr=him for trunk and 1.8 branch.  I'll work on a 1.8.0 branch patch.
Attachment #221761 - Flags: superreview?(brendan)
Attachment #221761 - Flags: superreview+
Attachment #221761 - Flags: review?(mrbkap)
Attachment #221761 - Flags: approval-branch-1.8.1?(jst)
(Assignee)

Comment 14

11 years ago
Created attachment 225472 [details] [diff] [review]
1.8.0 branch version

Same as the other, but with only API addition; the only changes from the other patch are in jsdbgapi.{h,cpp}, so I figure jseng owner review is enough.
Attachment #225472 - Flags: review?(shaver)
Comment on attachment 221761 [details] [diff] [review]
With Brendan's issues addressed

>+ * Get the script filename flags for the script.  If the script
>+ * doesn't have a filename, return JSFILENAME_NULL.

Too many uses of "script" there, lose the first one ("Get the filename flags for the script.").

>+                // else don't create XPCNativeWrappers, I guess.
>+  // No script means we should bypass, I guess?  That's what the old code did.

No guessing, either we like the original design, or we fix it here, or file bugs and cite them in FIXME comments.

Nit: func and funObj are used for the same kind of pointer, unify on funobj or callee to match other code in xpconnect and js?

Main suggestion: why not add mFunObj or mCallee to XPCCallContext and avoid the need to add yet another parameter in lots of places?

/be
(Assignee)

Comment 16

11 years ago
> or file bugs and cite them in FIXME comments.

I'll file bugs to investigate that stuff as needed.

I can unify on funObj, sure.

> why not add mFunObj or mCallee to XPCCallContext

I want something provably correct that I won't mind landing on the branches.  I don't have a good feel for when such members of an XPCCallContext could get clobbered (e.g. whether any of this code can be reentered and when); to feel confident about it I'd need to understand a lot more about control flow through XPConnect than I do now or will in time for any of the 1.8.0 security releases I've wanted to target this at.

If someone can prove to me that this would be a safe change, I'm willing to do it, I guess.  Or I can file a followup bug on trunk to do it; whichever folks prefer.
(In reply to comment #16)
> 
> I can unify on funObj, sure.

"funobj" all lower-case, a grep in xpconnect/src shows, is the canonical spelling.

> > why not add mFunObj or mCallee to XPCCallContext
> 
> I want something provably correct that I won't mind landing on the branches.  I
> don't have a good feel for when such members of an XPCCallContext could get
> clobbered (e.g. whether any of this code can be reentered and when);

XPCCallContext is storage class auto, LIFO allocated on the thread stack, not subject to clobbering.  Adding a member to it is equivalent to adding a local variable that can be referenced outside of C++ function scope, which is why it wins for this case -- only some calls need to pass the funobj through.

/be
(Assignee)

Comment 18

11 years ago
Created attachment 225544 [details] [diff] [review]
Now with an mCallee member
Attachment #221761 - Attachment is obsolete: true
Attachment #225472 - Attachment is obsolete: true
Attachment #225544 - Flags: superreview?(brendan)
Attachment #225544 - Flags: review?(jst)
Attachment #225544 - Flags: approval-branch-1.8.1?(jst)
Attachment #221761 - Flags: approval-branch-1.8.1?(jst)
Attachment #225472 - Flags: review?(shaver)
(Assignee)

Comment 19

11 years ago
Created attachment 225545 [details] [diff] [review]
Now with an mCallee member for real
Attachment #225544 - Attachment is obsolete: true
Attachment #225545 - Flags: superreview?(brendan)
Attachment #225545 - Flags: review?(jst)
Attachment #225545 - Flags: approval-branch-1.8.1?(jst)
Attachment #225544 - Flags: superreview?(brendan)
Attachment #225544 - Flags: review?(jst)
Attachment #225544 - Flags: approval-branch-1.8.1?(jst)
(Assignee)

Comment 20

11 years ago
Created attachment 225546 [details] [diff] [review]
1.8.0.x branch version

The only difference here is the jsdbgapi code.
Attachment #225546 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Attachment #225545 - Flags: review?(jst) → review?(mrbkap)

Updated

11 years ago
Attachment #225545 - Flags: review?(mrbkap) → review+
Comment on attachment 225545 [details] [diff] [review]
Now with an mCallee member for real

>+  // Check what the script calling us looks like
>+  JSScript *script = nsnull;
>+  JSStackFrame *fp = cx->fp;
>+  while(!script && fp)
>+  {
>+    script = fp->script;
>+    fp = fp->down;
>+  }

Nit: prevailing style in XPCNativeWrapper.cpp is jst-ish/dom-like: opening brace on same line as control keyword and condition, not on its own line.

sr=me with that -- and I am happy to branch-approve too, but if you want to wait till jst is back, I won't object.

/be
Attachment #225545 - Flags: superreview?(brendan) → superreview+
Comment on attachment 225546 [details] [diff] [review]
1.8.0.x branch version

Did we resolve to break ABI (not that anyone cares about jsdbgapi.h stuff added only for XPCNativeWrapper usage) in 1.8.1?  Ok by me, just checking.  I was considering restoring the lost JSFunctionSpec ABI compat for JS 1.7 in Gecko 1.8.1.

If we want to keep compat, then this is good for both branches, obviously.

/be
Attachment #225546 - Flags: review?(shaver)
Attachment #225546 - Flags: review?(brendan)
Attachment #225546 - Flags: review+
Comment on attachment 225546 [details] [diff] [review]
1.8.0.x branch version

ABI is broken by the JSFunctionSpec changes that will come as part of the JS1.7 -> branch landing, no?  I thought we were already committed to that.  If I'm mistaken and we're not, then we should probably take the 1.8.0 patch on 1.8.1 as well.

(Sort of a shame that we're breaking ABI in 1.8 rather than 1.9, but I think we'll probably live.  Especially if we reach out early to extension and plugin authors, which I guess is my job!)

r=shaver on the assumption (!) that I'm not mistaken above about JSFunctionSpec.
Attachment #225546 - Flags: review?(shaver) → review+
Comment on attachment 225546 [details] [diff] [review]
1.8.0.x branch version

Dammit, I need to read better.  Sorry, yeah, if we're restoring the JSFunctionSpec stuff, we should take the 1.8.0 patch on 1.8.  So let's just take it anyway, rather than have to change it later.

181=shaver
Attachment #225546 - Flags: approval-branch-1.8.1+
(Assignee)

Updated

11 years ago
Attachment #225545 - Flags: approval-branch-1.8.1?(jst)
(Assignee)

Comment 25

11 years ago
Created attachment 225636 [details] [diff] [review]
Trunk patch updated to all comments.
Attachment #225545 - Attachment is obsolete: true
(Assignee)

Comment 26

11 years ago
Created attachment 225637 [details] [diff] [review]
1.8/1.8.0 branch patch updated to all comments
Attachment #225546 - Attachment is obsolete: true
(Assignee)

Comment 27

11 years ago
Checked in on trunk and 1.8.1 branch.  Let the baking begin!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 338114
Comment on attachment 225637 [details] [diff] [review]
1.8/1.8.0 branch patch updated to all comments

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #225637 - Flags: approval1.8.0.5+
(Assignee)

Comment 29

11 years ago
Checked in for 1.8.0.5.
Keywords: fixed1.8.0.5
(Assignee)

Updated

11 years ago
Flags: blocking1.9a1?
Flags: blocking1.8.1?

Comment 30

11 years ago
Is there a way to test these changes on the 1.8.0 branch to verify the fix?
Whiteboard: [need testcase]
(Assignee)

Comment 31

11 years ago
I tested by installing adblock, then editing the code it installed to output what node it actually got from our C++.  You might be able to reproduce with the "minimal testcase" from bug 336875 and some changes to the code to see whether you're actually getting an XPCNativeWrapper.

Updated

9 years ago
Blocks: 467520
You need to log in before you can comment on or make changes to this bug.