Closed
Bug 779724
Opened 12 years ago
Closed 12 years ago
make source retention more fine grained
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: Benjamin, Assigned: Benjamin)
References
Details
Attachments
(2 files)
12.64 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
17.71 KB,
patch
|
jorendorff
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Assignee: general → bpeterson
Attachment #648224 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•12 years ago
|
||
The rooting is nessecary but bogus in this patch.
Assignee | ||
Comment 3•12 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
Comment 4•12 years ago
|
||
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•12 years ago
|
Attachment #648226 -
Flags: review?(jorendorff)
Updated•12 years ago
|
Attachment #648226 -
Flags: superreview?(jst)
Comment 5•12 years ago
|
||
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•12 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 7•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3a07fc6887 https://hg.mozilla.org/integration/mozilla-inbound/rev/de346dd6e48a
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc3a07fc6887 https://hg.mozilla.org/mozilla-central/rev/de346dd6e48a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•