Last Comment Bug 725576 - serialize principals only once per top-level script
: serialize principals only once per top-level script
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: 643967
  Show dependency treegraph
 
Reported: 2012-02-08 22:49 PST by Igor Bukanov
Modified: 2012-05-31 15:19 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
-
wontfix


Attachments
v1 (16.13 KB, patch)
2012-02-09 05:14 PST, Igor Bukanov
mrbkap: review-
Details | Diff | Review
v2 (31.12 KB, patch)
2012-02-10 03:58 PST, Igor Bukanov
no flags Details | Diff | Review
v3 (39.29 KB, patch)
2012-02-10 14:44 PST, Igor Bukanov
no flags Details | Diff | Review
v4 (44.90 KB, patch)
2012-02-12 07:38 PST, Igor Bukanov
no flags Details | Diff | Review
v5 (44.86 KB, patch)
2012-02-16 14:47 PST, Igor Bukanov
no flags Details | Diff | Review
v6 (39.79 KB, patch)
2012-02-21 14:32 PST, Igor Bukanov
no flags Details | Diff | Review
v6 for real (39.07 KB, patch)
2012-02-22 00:40 PST, Igor Bukanov
no flags Details | Diff | Review
v7 (39.13 KB, patch)
2012-02-22 02:35 PST, Igor Bukanov
luke: review+
Details | Diff | Review

Description Igor Bukanov 2012-02-08 22:49:41 PST
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.
Comment 1 Igor Bukanov 2012-02-09 05:14:27 PST
Created attachment 595705 [details] [diff] [review]
v1

The patch implements that serialize once idea.
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-02-09 06:13:01 PST
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!
Comment 3 Igor Bukanov 2012-02-10 03:58:00 PST
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.
Comment 4 Igor Bukanov 2012-02-10 14:44:03 PST
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.
Comment 5 Igor Bukanov 2012-02-12 07:38:49 PST
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.
Comment 6 Igor Bukanov 2012-02-16 14:47:00 PST
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.
Comment 7 Igor Bukanov 2012-02-21 07:07:46 PST
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.
Comment 8 Igor Bukanov 2012-02-21 14:32:54 PST
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.
Comment 9 Igor Bukanov 2012-02-22 00:40:43 PST
Created attachment 599513 [details] [diff] [review]
v6 for real

The previous attachment had a wrong patch - I forgot to refresh it.
Comment 10 Igor Bukanov 2012-02-22 02:35:28 PST
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.
Comment 11 Igor Bukanov 2012-02-23 08:29:02 PST
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.
Comment 12 Luke Wagner [:luke] 2012-02-23 09:07:16 PST
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...)
Comment 13 Igor Bukanov 2012-02-23 10:52:34 PST
(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.
Comment 14 Igor Bukanov 2012-02-23 11:10:29 PST
(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 Luke Wagner [:luke] 2012-02-23 12:11:00 PST
Sweet
Comment 17 Marco Bonardo [::mak] 2012-02-24 02:37:26 PST
https://hg.mozilla.org/mozilla-central/rev/c2c2a5b0c313
Comment 18 [On PTO until 6/29] 2012-04-10 13:45:24 PDT
Verified for trunk based on passing checked in tests since we have no manual cases / steps for this.
Comment 19 David Mandelin [:dmandelin] 2012-05-31 11:19:29 PDT
Luke, would the patch apply to ESR10? If not, could it reasonably be backported?
Comment 20 Luke Wagner [:luke] 2012-05-31 11:23:26 PDT
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).

Note You need to log in before you can comment on or make changes to this bug.