Closed
Bug 1150513
Opened 10 years ago
Closed 10 years ago
Fix a few places where we can relazify a function while we're working with its script
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 1 obsolete file)
This is dumb. This can happen in CloneFunctionAndScript (which blocks 679939), in CloneFunctionObject (which has a workaround for it of regetting the script after clone), in CloneFunctionScript (but only if scope is non-null).
Should protect against this.
Assignee | ||
Comment 1•10 years ago
|
||
Actually, CloneFunctionScript is safe, because clone is same-compartment with cx so can't be relazified on GC.
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8587459 -
Flags: review?(luke)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8587486 -
Flags: review?(luke)
Assignee | ||
Updated•10 years ago
|
Attachment #8587459 -
Attachment is obsolete: true
Attachment #8587459 -
Flags: review?(luke)
Comment 4•10 years ago
|
||
Comment on attachment 8587486 [details] [diff] [review]
While we're working with a function's script (e.g. cloning it), prevent that function getting relazified
Review of attachment 8587486 [details] [diff] [review]:
-----------------------------------------------------------------
Great!
::: js/src/jsfun.cpp
@@ +667,3 @@
> if (!srcScript)
> return nullptr;
> RootedScript clonedScript(cx, CloneScript(cx, enclosingScope, clone, srcScript, polluted));
Do you think you could add an assert to CloneScript that
!script->isRelazifiable() || script->compartment()->hasBeenEntered()
? (I'm not actually sure of the full set of conditions that inhibit relazification; you'd hope it was just isRelazifiable.)
::: js/src/jsscript.h
@@ +1013,5 @@
> + // Do not relazify this script. This is used by the relazify() testing
> + // function for scripts that are on the stack and also by the
> + // AutoDelazify RAII class. Usually we don't relazify functions in
> + // compartments with scripts on the stack, but the relazify() function
> + // overrides that, and sometimes we're working with a cross-compartment
Despite the first sentence of the para, my mind read 'relazify()' as 'JSFunction::relazify()' and I was confused. Could you explicitly qualify as 'the relazify() testing function'?
@@ +1698,5 @@
>
> void markChildren(JSTracer* trc);
> +
> + // A helper class to prevent relazification of the script it's got
> + // while it's holding on to it. This class automatically roots
s/the script it's got while it's holding on to it/the given script/
@@ +1728,5 @@
> + holdScript(fun);
> + }
> +
> + operator JS::HandleScript() const { return script_; }
> + operator bool() const { return script_; }
I think we can use explicit operator bool now.
Attachment #8587486 -
Flags: review?(luke) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Did all of the above. There are other conditions that can inhibit relazification, but they're dynamic ones (e.g. "debugger is attached"), not static ones based on code flow, so callers of CloneScript can't rely on them holding.
Assignee | ||
Comment 6•10 years ago
|
||
OK, so...
The asserts I added in CloneScript are failing, and I'm not sure how to fix them.
The problem is that what we really care about is whether a given _function_ can be relazified. But CloneScript() doesn't have access to that function.
Specifically, in CloneFunctionAndScript we care about "srcFun" being relazified, but the compartment we're in is that of the clone (so "fun" in terms of CloneScript).
Conversely, in CloneFunctionScript we care about "clone" being relazified (which is the "fun" argument to CloneScript()). We don't care what happens with "original". Here we're in the compartment of "clone" so it's all OK.
But fundamentally, CloneScript just can't tell what the caller is doing in terms of which function's lazy state it cares about: the pre-clone function or post-clone. So I'm just going to take the assert out for now, I think....
Assignee | ||
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•