make nsContentUtils::GetDocumentFromCaller not depend on slow-native constructors

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

Posted patch patchSplinter Review
GetDocumentFromCaller is used to determine new elements' documents using the callee's document when creating option, image, and audio elements with 'new'.  This is currently implemented by looking at the top stack frame.  With bug 581263, constructors (like XPC_WN_Helper_Construct) won't get stack frames, hence we need a different way to find the callee.  This patch uses the thread's current XPCCallContext.  While it would make sense to use ccx->GetArgv()[-2], with fast natives, this aliases the rval.  Fortunately, the callee object is saved separately in XPCCallContext::GetFlattedJSObject (for the cases we care about: XPC_WN_Helper_{Call,Construct}).
Attachment #467626 - Flags: review?
Comment on attachment 467626 [details] [diff] [review]
patch

Ah, I hadn't noticed that my r?peterv was turned into r? for lack of ':'.
Attachment #467626 - Flags: review? → review?(peterv)
review ping
Attachment #467626 - Flags: review?(peterv) → review?(jst)
(In reply to comment #0)
> Fortunately, the callee object is
> saved separately in XPCCallContext::GetFlattedJSObject (for the cases we care
> about: XPC_WN_Helper_{Call,Construct}).

Is that really the case? I thought that just returned the wrapper's object from the wrapper in obj. How is that the same as "the document where the calling code came from"?
(In reply to comment #3)
> Is that really the case? I thought that just returned the wrapper's object from
> the wrapper in obj. How is that the same as "the document where the calling
> code came from"?

XPC_WN_Helper_{Call,Construct} passes the callee as 'obj'.  I think I could/should add an assert to XPC_WN_Helper_{Call,Construct} that JS_CALLEE(cx, vp) == ccx.GetFlattenedJSObject().
Comment on attachment 467626 [details] [diff] [review]
patch

r=jst with the assert added, but I also think we should create a mochitest that ensures that new Option().ownerDocument (etc) is correct in a cross frame call.
Attachment #467626 - Flags: review?(jst) → review+
Please check performance numbers closely when landing this, removing those XPCCallContexts used to be a performance win :-/. We really should try to redo how this is handled, ideally we'd get the document from obj in nsDOMConstructor::Construct/Call and pass it along from there to the constructors.
(In reply to comment #6)
Sure, although stepping through it in the debugger it seems like the path is slow enough already to mask the overhead of an XPCCallContext.  We'll see though.

http://hg.mozilla.org/tracemonkey/rev/d008c236ac46
Whiteboard: fixed-in-tracemonkey
This landed on m-c already, can we close this bug now?
Yes.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.