Closed Bug 725576 Opened 9 years ago Closed 9 years ago

serialize principals only once per top-level script

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla13
Tracking Status
firefox13 --- fixed
firefox-esr10 - wontfix

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
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-
Attached patch v2 (obsolete) — Splinter Review
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)
Attached patch v3 (obsolete) — Splinter Review
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)
Attached patch v4 (obsolete) — Splinter Review
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)
Attached patch v5 (obsolete) — Splinter Review
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)
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)
Attached patch v6 (obsolete) — Splinter Review
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)
Attached patch v6 for real (obsolete) — Splinter Review
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)
Attached patch v7Splinter Review
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)
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 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+
(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.
(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.
Sweet
https://hg.mozilla.org/mozilla-central/rev/c2c2a5b0c313
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Blocks: 643967
Verified for trunk based on passing checked in tests since we have no manual cases / steps for this.
Status: RESOLVED → VERIFIED
Luke, would the patch apply to ESR10? If not, could it reasonably be backported?
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).
You need to log in before you can comment on or make changes to this bug.