1467022 - JSFunction::hasScript needs comment about the relation between JSFunction::hasUncompletedScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

JSFunction::hasScript doesn't check the existence of the pointer to JSScript (u.scripted.s.script_), which is checked inside JSFunction::hasUncompletedScript.

At least JSFunction::hasScript should be renamed to something else that clarifies what actually it's checking.
and also might be better checking the JSFunction::hasUncompletedScript's condition.

Comment 1

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> At least JSFunction::hasScript should be renamed to something else that
> clarifies what actually it's checking.

or add some comment that hasScript() implies !hasUncompletedScript() for live JSFunctions (which is linked from live objects),
instead of dead JSFunction linked from JSScript/LazyScript which is gathered by scanning arenas, in Debugger.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> or add some comment that hasScript() implies !hasUncompletedScript() for
> live JSFunctions (which is linked from live objects),
> instead of dead JSFunction linked from JSScript/LazyScript which is gathered
> by scanning arenas, in Debugger.

That makes sense. The JITs inline hasScript (they check the INTERPRETED flag) and we don't want to null-check script_ there. It would be good to check the same thing in C++ and JIT code.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Add comment about the script field to JSFunction::hasScript, and refer JSFunction::hasUncompletedScript.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8995053 [details] [diff] [review]
Add comment about the script field to JSFunction::hasScript, and refer JSFunction::hasUncompletedScript.

Review of attachment 8995053 [details] [diff] [review]:
-----------------------------------------------------------------

👍🏼

::: js/src/vm/JSFunction.h
@@ +266,5 @@
> +    // This method doesn't check the non-nullness of u.scripted.s.script_,
> +    // because it's guaranteed to be non-null when this has INTERPRETED flag,
> +    // for live JSFunctions.
> +    //
> +    // When this JSFunction instance if reached via GC iteration, the above

s/if/is/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/090236db7f10
Add comment about the script field to JSFunction::hasScript, and refer JSFunction::hasUncompletedScript. r=jandem DONTBUILD, comment-only

Comment 6

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/090236db7f10