make source retention more fine grained

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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)

Comment 1

5 years ago
Created attachment 648224 [details] [diff] [review]
add source retention policy to CompileOptions
Assignee: general → bpeterson
Attachment #648224 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
Created attachment 648226 [details] [diff] [review]
use finer-grain source policies in the browser

The rooting is nessecary but bogus in this patch.
(Assignee)

Comment 3

5 years ago
(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
(Assignee)

Updated

5 years ago
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+
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 6

5 years ago
(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+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3a07fc6887
https://hg.mozilla.org/integration/mozilla-inbound/rev/de346dd6e48a

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/bc3a07fc6887
https://hg.mozilla.org/mozilla-central/rev/de346dd6e48a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.