Closed
Bug 337095
Opened 19 years ago
Closed 18 years ago
[FIX]Sort out XPCNativeWrapper behavior for calls from C++ to JS
Categories
(Core :: XPConnect, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: fixed1.8.0.5, fixed1.8.1, Whiteboard: [need testcase])
Attachments
(2 files, 6 obsolete files)
21.05 KB,
patch
|
Details | Diff | Splinter Review | |
21.95 KB,
patch
|
dveditz
:
approval1.8.0.5+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Attachment #221376 -
Flags: superreview?(brendan)
Attachment #221376 -
Flags: review?(jst)
Comment 2•19 years ago
|
||
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•19 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.
Comment 4•19 years ago
|
||
(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•19 years ago
|
||
Assignee: dbradley → bzbarsky
Status: NEW → ASSIGNED
Attachment #221761 -
Flags: superreview?(brendan)
Attachment #221761 -
Flags: review?(jst)
Assignee | ||
Comment 6•19 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•19 years ago
|
Attachment #221376 -
Attachment is obsolete: true
Attachment #221376 -
Flags: superreview?(brendan)
Attachment #221376 -
Flags: review?(jst)
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.9a1?
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
Comment 7•18 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•18 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?
Comment 9•18 years ago
|
||
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•18 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•18 years ago
|
Attachment #221761 -
Flags: review?(mrbkap)
Comment 11•18 years ago
|
||
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 12•18 years ago
|
||
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•18 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•18 years ago
|
||
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 15•18 years ago
|
||
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•18 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.
Comment 17•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
The only difference here is the jsdbgapi code.
Attachment #225546 -
Flags: review?(brendan)
Assignee | ||
Updated•18 years ago
|
Attachment #225545 -
Flags: review?(jst) → review?(mrbkap)
Updated•18 years ago
|
Attachment #225545 -
Flags: review?(mrbkap) → review+
Comment 21•18 years ago
|
||
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 22•18 years ago
|
||
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 23•18 years ago
|
||
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 24•18 years ago
|
||
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•18 years ago
|
Attachment #225545 -
Flags: approval-branch-1.8.1?(jst)
Assignee | ||
Comment 25•18 years ago
|
||
Attachment #225545 -
Attachment is obsolete: true
Assignee | ||
Comment 26•18 years ago
|
||
Attachment #225546 -
Attachment is obsolete: true
Assignee | ||
Comment 27•18 years ago
|
||
Checked in on trunk and 1.8.1 branch. Let the baking begin!
Comment 28•18 years ago
|
||
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 | ||
Updated•18 years ago
|
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Comment 30•18 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•18 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•