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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

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}).

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 467626 [details] [diff] [review]
patch

Ah, I hadn't noticed that my r?peterv was turned into r? for lack of ':'.

Comment 2

[Luke Wagner [:luke]]

review ping

Comment 3

[Peter Van der Beken [:peterv]]

(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"?

Comment 4

[Luke Wagner [:luke]]

(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 5

[Johnny Stenback (:jst)]

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.

Comment 6

[Peter Van der Beken [:peterv]]

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.

Comment 7

[Luke Wagner [:luke]]

(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

Comment 8

[Johnny Stenback (:jst)]

This landed on m-c already, can we close this bug now?

Comment 9

[Luke Wagner [:luke]]

Yes.