Closed Bug 779724 Opened 9 years ago Closed 9 years ago

make source retention more fine grained

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: Benjamin, Assigned: Benjamin)

References

Details

Attachments

(2 files)

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.
Assignee: general → bpeterson
Attachment #648224 - Flags: review?(jorendorff)
The rooting is nessecary but bogus in this patch.
(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
Depends on: 779865
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.
Attachment #648224 - Flags: review?(jorendorff) → review+
Attachment #648226 - Flags: review?(jorendorff)
Attachment #648226 - Flags: superreview?(jst)
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.
Attachment #648226 - Flags: review?(jorendorff) → review+
(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 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.
Attachment #648226 - Flags: superreview?(jst) → superreview+
https://hg.mozilla.org/mozilla-central/rev/bc3a07fc6887
https://hg.mozilla.org/mozilla-central/rev/de346dd6e48a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.