Last Comment Bug 779724 - make source retention more fine grained
: make source retention more fine grained
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Benjamin Peterson
:
Mentors:
Depends on: 779865
Blocks: 776741
  Show dependency treegraph
 
Reported: 2012-08-01 22:29 PDT by :Benjamin Peterson
Modified: 2012-08-08 09:35 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add source retention policy to CompileOptions (12.64 KB, patch)
2012-08-01 22:44 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review
use finer-grain source policies in the browser (17.71 KB, patch)
2012-08-01 22:45 PDT, :Benjamin Peterson
jorendorff: review+
jst: superreview+
Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-08-01 22:29:20 PDT
JSOPTION_ONLY_CNG_SOURCE is icky. It globally implicitly applies to an entire context and doesn't easily allow changing source rentention for a certain compile call. Sometimes it doesn't get set on ad-hoc JSContexts it really should be. Source retention should be controlled by a flag or enum in CompileOptions.
Comment 1 :Benjamin Peterson 2012-08-01 22:44:10 PDT
Created attachment 648224 [details] [diff] [review]
add source retention policy to CompileOptions
Comment 2 :Benjamin Peterson 2012-08-01 22:45:18 PDT
Created attachment 648226 [details] [diff] [review]
use finer-grain source policies in the browser

The rooting is nessecary but bogus in this patch.
Comment 3 :Benjamin Peterson 2012-08-01 22:47:03 PDT
(In reply to Benjamin Peterson from comment #2)
> Created attachment 648226 [details] [diff] [review]
> use finer-grain source policies in the browser
> 
> The rooting is nessecary but bogus in this patch.

*necessary
Comment 4 Jason Orendorff [:jorendorff] 2012-08-03 09:15:51 PDT
Comment on attachment 648224 [details] [diff] [review]
add source retention policy to CompileOptions

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +264,1 @@
>          return NULL;

Style nit: I like this style:

    if (condition) {
        if (!action())
            return NULL;
    }

but if you want to write it using &&, house style requires curly braces around the body because of the multiline condition.

::: js/src/jsscript.cpp
@@ +641,5 @@
>              script->filename = enclosingScript->filename;
>      }
>  
> +    if (scriptBits & (1 << OwnSource) && !script->scriptSource()->performXDR<mode>(xdr))
> +        return false;

Nit: overparenthesize the &-expression against &&.

::: js/src/jsscript.h
@@ +990,5 @@
>      uint32_t length_;
>      uint32_t compressedLength_;
>      bool marked:1;
>      bool onRuntime_:1;
> +    bool sourceRetrievable_:1;

Needs a comment. The thing to mention here is that there are actually three states. If sourceRetrievable is true, the sourceHook can be called, no problem. If sourceRetrievable is false, either we already have the source, or we don't have it and can't get it.
Comment 5 Jason Orendorff [:jorendorff] 2012-08-03 13:50:21 PDT
Comment on attachment 648226 [details] [diff] [review]
use finer-grain source policies in the browser

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

r=me with the one comment addressed.

::: dom/base/nsIScriptContext.h
@@ +125,5 @@
>                                   const char* aURL,
>                                   PRUint32 aLineNo,
>                                   PRUint32 aVersion,
> +                                 nsScriptObjectHolder<JSScript>& aScriptObject,
> +                                 bool aSaveSource = false) = 0;

Doesn't this change the default behavior from SAVE to LAZY? Isn't that bad?

The other places where you've changed a specific Compile call from SAVE to LAZY seem fine, judging on a case-by-case basis. For content scripts, though, I think we should still SAVE.
Comment 6 :Benjamin Peterson 2012-08-03 13:57:03 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Doesn't this change the default behavior from SAVE to LAZY? Isn't that bad?

It preserves the behavior that is present now, actually.

> 
> The other places where you've changed a specific Compile call from SAVE to
> LAZY seem fine, judging on a case-by-case basis. For content scripts,
> though, I think we should still SAVE.

You never call CompileScript on a content script, though.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2012-08-07 09:30:54 PDT
Comment on attachment 648226 [details] [diff] [review]
use finer-grain source policies in the browser

- In dom/base/nsIScriptContext.h

   virtual nsresult CompileScript(const PRUnichar* aText,
                                  PRInt32 aTextLength,
                                  nsIPrincipal* aPrincipal,
                                  const char* aURL,
                                  PRUint32 aLineNo,
                                  PRUint32 aVersion,
-                                 nsScriptObjectHolder<JSScript>& aScriptObject) = 0;
+                                 nsScriptObjectHolder<JSScript>& aScriptObject,
+                                 bool aSaveSource = false) = 0;

This is a binary incompatible change to this interface, please update the IID for this interface before landing.

sr=jst with that.

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