1334091 - XDR enclosingScript is only used to get/check the sourceObject.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Bug 1316081 is currently encoding the source 215 times because we encode the source for each delazified function.  The reason being that we do not provide an enclosingScript when the function is delazified.

We could work-around this issue by giving the encoded script as being the enclosingScript, but this would be ugly.  At the moment, only a single compartment assertion checks more than the source object, and this is a tautology by the way the XDR recursive decent is built.

I suggest, as part of this bug, that we replace the enclosingScript argument by a sourceObject argument, which would better fit the way it is used in XDR functions.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: XDR function use the sourceObject instead of the enclosingScript as argument.

This patch replaces the enclosingScript argument by the sourceOBject
argument.

In addition to make things easier to understand (1), this also catch the issue
seen in Bug 1316081 with the test cases added with it, as the incremental
encode will the source multiple time, and the decoder asserts that the
sourceObject is not set when the OwnSource flag is present.

(1) I honestly wondered for a while what the enclosingScript was used for the
first time I read about XDR code.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8830719 [details] [diff] [review]
XDR function use the sourceObject instead of the enclosingScript as argument.

Review of attachment 8830719 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the cleanup.

One thing I'm unclear on is how JS::EncodeInterpretedFunction is used. What's being passed to it from XPConnect? What if I pass a delazified inner function to it? That'll end up being considered OwnSource, right?

::: js/src/jsscript.cpp
@@ +487,5 @@
>      if (!xdr->codeUint32(&scriptBits))
>          return false;
>  
> +    MOZ_ASSERT(!!(scriptBits & (1 << OwnSource)) == !sourceObject);
> +    RootedScriptSource sourceObj(cx, sourceObject);

The convention I like for these things is the argument gets an "Arg", so

RootedScriptSource sourceObject(cx, sourceObjectArg);

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #2)
> One thing I'm unclear on is how JS::EncodeInterpretedFunction is used.
> What's being passed to it from XPConnect? What if I pass a delazified inner
> function to it? That'll end up being considered OwnSource, right?

From what I have been told, the only user of JS::EncodeInterpreterFunction is XBL [1].  IIRC, XBL contains some sort of JS functions which are encoded in xml[2]. They are already delazified by the CompileFunction call.

Yes, the XDR buffer of these function will contain the OwnSource flag, I honestly have not tried to challenge the need for the source and only kept the original behaviour.

[1] http://searchfox.org/mozilla-central/source/dom/xbl/nsXBLSerialize.cpp#23
[2] http://searchfox.org/mozilla-central/source/dom/xbl/nsXBLProtoImplMethod.h#37

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f9ff384f30d
XDR function use the sourceObject instead of the enclosingScript as argument. r=shu

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f9ff384f30d