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)

x86
macOS
defect
Not set
normal

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.
Actually, CloneFunctionScript is safe, because clone is same-compartment with cx so can't be relazified on GC.
Attachment #8587459 - Attachment is obsolete: true
Attachment #8587459 - Flags: review?(luke)
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+
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.
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....
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.

Attachment

General

Created:
Updated:
Size: