serialize principals only once per top-level script

VERIFIED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(firefox13 fixed, firefox-esr10- wontfix)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

5 years ago
Currently JS_XDRScript serializes principals for each its nested function in addition to the top-level script. Since principals in the nested function are the same as for the script, we should fix and do the serialization only once per top-level serialization.
(Assignee)

Comment 1

5 years ago
Created attachment 595705 [details] [diff] [review]
v1

The patch implements that serialize once idea.
Assignee: general → igor
Attachment #595705 - Flags: review?(mrbkap)
Comment on attachment 595705 [details] [diff] [review]
v1

I really like this approach, but r- because of JS_XDRFunctionObject which is (as far as I can tell) provides a way to XDR a script without calling JS_XDRScript. Igor is on the case!
Attachment #595705 - Flags: review?(mrbkap) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 596011 [details] [diff] [review]
v2

That approach with separated principals init phase lead to unnecessary code duplication after accounting for JS_XDRFunctionObject.

So this patch adds a new structure, ScriptShared, that holds the principals and changes js_XDRScript/js_XDRFunctionObject to take explicit ScriptShared argument. This allowed to unify handling of JSScript::filename and principals, eliminate XDRScriptState and simplify CloneScipt implementation.

In the patch I dropped the requirement that originPrincipals must be null if principals is null. That did not allow to simplify anything especially since with the patch CloneScript uses the common principal initialization code shared with xdr decoding. So the patch leaves up to embedding to decide if null principal and non-null origin is a valid configuration.
Attachment #595705 - Attachment is obsolete: true
Attachment #596011 - Flags: review?(mrbkap)
(Assignee)

Comment 4

5 years ago
Created attachment 596173 [details] [diff] [review]
v3

v2 has a bug when checking for presence of principal transcode callback. Our mochitest caught it, but that clearly points to deficiency of our api tests. So in v3 I added tests to check various aspects of principal serialization/deserialization.
Attachment #596011 - Attachment is obsolete: true
Attachment #596011 - Flags: review?(mrbkap)
Attachment #596173 - Flags: review?(mrbkap)
(Assignee)

Comment 5

5 years ago
Created attachment 596477 [details] [diff] [review]
v4

The new patch improves the test coverage plus changes json test not to output anything if the test is successful.
Attachment #596173 - Attachment is obsolete: true
Attachment #596173 - Flags: review?(mrbkap)
Attachment #596477 - Flags: review?(mrbkap)
(Assignee)

Comment 6

5 years ago
Created attachment 598007 [details] [diff] [review]
v5

In v4 I forgot in the XDR test to call JSPRINCIPALS_HOLD for the decoded principal. That was not visible as the destroy method were empty. In the new patch I fix that and replace the destroy method with NULL so the code will crash on unexpected principal release.
Attachment #596477 - Attachment is obsolete: true
Attachment #596477 - Flags: review?(mrbkap)
Attachment #598007 - Flags: review?(mrbkap)
(Assignee)

Comment 7

5 years ago
Comment on attachment 598007 [details] [diff] [review]
v5

In view of changes from the bug 728250 this patch goes to the wrong direction. So I create smaller one that should allow to fix the problems on branches if that becomes necessary while minimizing the changes for the bug 728250.
Attachment #598007 - Attachment is obsolete: true
Attachment #598007 - Flags: review?(mrbkap)
(Assignee)

Comment 8

5 years ago
Created attachment 599371 [details] [diff] [review]
v6

Here is that smaller patch. As before, XDRScript just copies during the decoding the principals. But the patch avoids separated state objects and just stores principals and the filename in JSXDRState. Then principals are transcoded using couple of helpers in jsxdrapi.cpp.
Attachment #599371 - Flags: review?(mrbkap)
(Assignee)

Comment 9

5 years ago
Created attachment 599513 [details] [diff] [review]
v6 for real

The previous attachment had a wrong patch - I forgot to refresh it.
Attachment #599371 - Attachment is obsolete: true
Attachment #599371 - Flags: review?(mrbkap)
Attachment #599513 - Flags: review?(mrbkap)
(Assignee)

Comment 10

5 years ago
Created attachment 599544 [details] [diff] [review]
v7

The previous version has an interesting problem. In the patch I added a helper class, AutoDropPrincipals, to jsxdrapi.cpp. However a class with the same name was also defined in jsapi-tests/testCloneScript.cpp. On 64 bit Linux that resulted in the definition for the constructor and destructor for the test class to be used for the helper class. There were no warnings from the compiler about that.

So in the new version I have renamed the class and, just to be sure, also put the helper class in jsxdrapi.cpp into the anonymous namespace.
Attachment #599513 - Attachment is obsolete: true
Attachment #599513 - Flags: review?(mrbkap)
Attachment #599544 - Flags: review?(mrbkap)
(Assignee)

Comment 11

5 years ago
Comment on attachment 599544 [details] [diff] [review]
v7

luke, can you look at the patch? mrbkap do not have time due to the B2G work week.
Attachment #599544 - Flags: review?(mrbkap) → review?(luke)

Comment 12

5 years ago
Comment on attachment 599544 [details] [diff] [review]
v7

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

Nicely done!  It would be interesting to see if this produced a measurable decrease in total XDR time in browser startup.

::: js/src/jsapi-tests/testXDR.cpp
@@ +26,5 @@
> +    jschar *chars = static_cast<jschar *>(JS_malloc(cx, nchars * sizeof(jschar)));
> +    if (!chars)
> +        return NULL;
> +    JS_ALWAYS_TRUE(JS_DecodeBytes(cx, bytes, nbytes, chars, &nchars));
> +    JSScript *script = js::frontend::CompileScript(cx, obj, NULL, principals, originPrincipals,

Perhaps you can just add the jsapi?  There is no good reason for it to be missing.  That avoids the unexpected call to internals from jsapi-tests.

::: js/src/jsscript.cpp
@@ +722,1 @@
>          }

For the local reader, could you JS_ASSERT(script->principals, script->originPrincipals)?

@@ +1724,5 @@
>  CloneScript(JSContext *cx, JSScript *script)
>  {
>      JS_ASSERT(cx->compartment != script->compartment());
>  
> +    /* Serialize script. */

As long as you are fixing these //s, there are some more below.

::: js/src/jsxdrapi.cpp
@@ +532,5 @@
>      return JS_TRUE;
>  }
>  
> +static bool
> +XDRPrincipals(JSXDRState *xdr)

IIUC, any use of XDRPrincipals requires the caller to drop the principals held in xdr.  This seems like a delicate contract so perhaps we can explicate it with static types by having XDRPrincipals take an AutoDropXDRPrincipals (perhaps, for consistency, renamed to XDRPrincipalsGuard?) from which the JSXDRState object is derived.

@@ +563,5 @@
> +        if (xdr->mode == JSXDR_DECODE) {
> +            if (!scb || !scb->principalsTranscoder) {
> +                JS_ReportErrorNumber(xdr->cx, js_GetErrorMessage, NULL,
> +                                     JSMSG_CANT_DECODE_PRINCIPALS);
> +                return false;

Why is decoding w/o a transcoder a dynamically tested error but not encoding.  Seems like they would both be the same case.  Can they be?  (I'd prefer the assert, of course...)
Attachment #599544 - Flags: review?(luke) → review+
(Assignee)

Comment 13

5 years ago
(In reply to Luke Wagner [:luke] from comment #12)
> ::: js/src/jsxdrapi.cpp
> @@ +532,5 @@
> >      return JS_TRUE;
> >  }
> >  
> > +static bool
> > +XDRPrincipals(JSXDRState *xdr)
> 
> IIUC, any use of XDRPrincipals requires the caller to drop the principals
> held in xdr.  This seems like a delicate contract so perhaps we can
> explicate it with static types by having XDRPrincipals take an
> AutoDropXDRPrincipals (perhaps, for consistency, renamed to
> XDRPrincipalsGuard?) from which the JSXDRState object is derived.

I plan to remove XDRPrincipals and friends in the bug 728250 (part 2 there). So the changes here are temporary to minimize the changes in that bug.

> 
> @@ +563,5 @@
> > +        if (xdr->mode == JSXDR_DECODE) {
> > +            if (!scb || !scb->principalsTranscoder) {
> > +                JS_ReportErrorNumber(xdr->cx, js_GetErrorMessage, NULL,
> > +                                     JSMSG_CANT_DECODE_PRINCIPALS);
> > +                return false;
> 
> Why is decoding w/o a transcoder a dynamically tested error but not
> encoding.  Seems like they would both be the same case.  Can they be?  (I'd
> prefer the assert, of course...)

Again, wait for the bug 728250.
(Assignee)

Comment 14

5 years ago
(In reply to Luke Wagner [:luke] from comment #12)
> Perhaps you can just add the jsapi?  There is no good reason for it to be
> missing.  That avoids the unexpected call to internals from jsapi-tests.

I will add the missing API. But all those versions of compile script seems to suggest to refactor (in another bug!) the API bush into a parameter struct plus a single call to the compile method.

Comment 15

5 years ago
Sweet
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c2a5b0c313
https://hg.mozilla.org/mozilla-central/rev/c2c2a5b0c313
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

5 years ago
Blocks: 643967
status-firefox13: --- → fixed
Verified for trunk based on passing checked in tests since we have no manual cases / steps for this.
Status: RESOLVED → VERIFIED
tracking-firefox-esr10: --- → 14+
tracking-firefox-esr10: 14+ → 13+

Updated

5 years ago
tracking-firefox-esr10: 13+ → 14+
Luke, would the patch apply to ESR10? If not, could it reasonably be backported?

Comment 20

5 years ago
Naively applied, the patch conflicts terribly.  I'd also be scared to try to do a manual backporting since I believe this patch was landed in the middle of a slew of patches Igor wrote that totally redid XDR (more than just principals).

Updated

5 years ago
status-firefox-esr10: --- → wontfix
tracking-firefox-esr10: 14+ → -
You need to log in before you can comment on or make changes to this bug.