878484 - [jsdbg2] JSScript::sourceObject needn't call UncheckedUnwrap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

One of the patches for bug 637572 (Debugger.Source) introduces an accessor for a JSScript's sourceObject_ member that calls UnsafeUnwrap. I've gone through all the uses of JSScript::sourceObject to reassure myself that it was okay (and I believe it is). But as a general principle, it ought to be evident in the code that insecure constructs like UncheckedUnwrap are being used in constrained and local ways; returning its result from an accessor requires one to look at all the uses, which is the opposite of local.

Suppose JSScript::sourceObject didn't call UncheckedUnwrap? I think things might be perfectly fine. There are three kinds of uses for JSScript::sourceObject:

- In JSScript::scriptSource, we immediately dereference it to get the ScriptSource. Calling UncheckedUnwrap here would be fine; it's obvious the raw cross-compartment reference doesn't escape.

- In DebuggerScript_getSource, we immediately wrapSource it. Here, again, we can justify calling UncheckedUnwrap: it's essential for correctness that we only use direct ScriptSourceObject pointers, and never their wrappers, as keys when looking up Debugger.Source instances. It's true that Debugger.Source instances hold raw cross-compartment references, but that's by design: every Debugger.Foo class's instances are usually raw cross-compartment references, and the code handles them accordingly; such cross-privilege operation is Debugger's job.

- We use it in several places to copy the ScriptSourceObject pointer into a new JSScript: a child script, a clone, etc. Here there's no need for UncheckedUnwrap: JSScript::Create and friends would handle a cross-compartment wrapper just fine. JSScript::setSourceObject calls cx->compartment->wrap, which fully unwraps, and then rewraps.

Those are the only kinds of uses I've found. Nothing but the debugger, and JSScripts themselves, really wants to touch ScriptSourceObjects.

The annoying part is that this would entail changing a bunch of ScriptSourceObject * types to JSObject *, since JSScript::sourceObject might return either a ScriptSourceObject or a cross-compartment wrapper to one. But I think that's still better.

Comment 1

[Eddy Bruel [:ejpbruel]]

I agree. Since you opened a separate bug for this, I assume you want to do this in a followup patch?

Comment 2

[Jim Blandy :jimb]

(In reply to Eddy Bruel [:ejpbruel] from comment #1)
> I agree. Since you opened a separate bug for this, I assume you want to do
> this in a followup patch?

Right.

Comment 3

[Jim Blandy :jimb]

This was fixed as part of changeset a90070c1243c, for bug 637572.