254974 - introducing a function at the beginning of a function causes the program to change behavior

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[timeless]

steps:
1. run xpcshell or jsshell
2. paste the attached testcase into it.
note: you can *not* use load() to load the testcase as load optimizes away the
unused empty function resulting in expected results.

expected results:
DIV

actual results:
DIV[@id="test"]

I believe that adding the function seems to upset the regexp object used to fill
the array.

Comment 1

[timeless]

Attachment: code which confuses xpcshell/jsshell when pasted

Comment 2

[Brendan Eich [:brendan]]

Trunk-only bug, fallout from the mega-patch for bug 165201 and bug 169559.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix

All local var and arg properties in funobjs should be JSPROP_SHARED.  The one
whose define call is patched here was added for the JSOP_DEFLOCALVAR bytecode,
but was not made shared, so its slot could collide with a reserved (by
fun->nregexps, which isn't set till after the function body's code has been
generated, well after parse time) slot for a regexp.

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 155645 [details] [diff] [review]
proposed fix

That's a keeper.  Run the testsuite before committing, please!

Nice catch, timeless. *hat-tip*

Comment 5

[Brendan Eich [:brendan]]

Fixed, testsuite ran cleanly (same nine known errors, at least four of them bogus).

/be

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-254974.js

timeless, I assume it is ok with you to include this in the javascript test
library.

Comment 7

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-254974.js checked in.

Comment 8

[Bob Clary [:bc] (inactive)]

Brendan, the testcase I checked in relies on load to load the scripts. As
Timeless said, this prevents the bug from appearing. I tried creating the
functions via an eval on a string, but the bug didn't show its face. Do you have
any suggestions on how to make the bug manifest itself when |load|ed from jsDriver? 

Comment 9

[Brendan Eich [:brendan]]

It's just luck that the js shell (but not xpcshell, note well) does not enable
the JSOPTION_COMPILE_N_GO option when compiling and then executing standard
input, but does when Load'ing.  If JSFRAME_EVAL or JSFRAME_COMPILE_N_GO (the
latter induced by the corresponding JSOPTION_) is set, the compiler will emit a
JSOP_OBJECT for a regexp literal, optimizing away the need for a function slot
that would collide, given this bug (i.e., with the patch backed out), with the
unintended nested function variable slot.

I don't see a way to test this without a shell extension, or by piping stdin to
the shell.  Can't you do the latter from the perl driver, using a here document
even?  Perhaps we just need a bit more test framework here.  If not, let me know
and I'll make a shell extension (LoadSpecial or LoadDeoptimized or something).

/be

Comment 10

[Bob Clary [:bc] (inactive)]

I could munge jsDriver to use stdin perhaps as an option however that does not
appear to help in this case. doing cat test.js | js does not exhibit the bug. I
guess a shell extension is required.

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed.