449627 - TM: crash in js_FillPropertyCache

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Sayre]

Crash in-browser on the UA test used on mozilla.com

Comment 1

[Robert Sayre]

Attachment: backtraces for native stack, js stack

Comment 2

[Robert Sayre]

Attachment: test case

the detection code that triggers the crash

Comment 3

[Robert Sayre]

Same stack on gmail.com, after logging in.

Comment 4

[Robert Sayre]

Attachment: shell test case

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix

Rob, could you please test this? I've got other stuff in flight and could use the assist. Thanks,

/be

Comment 6

[Robert Sayre]

Now it crashes  at

            JS_ASSERT(OBJ_SCOPE(obj2)->shape == PCVCAP_SHAPE(entry->vcap));

a little further down. It doesn't assert... looks like obj2 doesn't have a sane value.

Comment 7

[Brendan Eich [:brendan]]

Attachment: better fix

TraceRecorder::test_property_cache cannot assume it will find the property, since we could be tracing an obj.prop evaluation where prop does not exist. We need to propagate "not found" (via PCVAL_NULL from test_property_cache, in turn via SPROP_INVALID_SLOT from test_property_cache_direct_slot) and in prop, synthesize an undefined value appropriately.

Also, various early return (abort) paths out of test_property_cache failed to drop prop (unlock obj2).

This fixes the bug, but with the current recording threshold of 1, the trace is aborted for unrelated reason (a JSOP_SETPROP for versionSearchString misses the cache on the second iteration that was filled on the first iteration; if we had recorded by the third, the cached pre-SETPROP shape would have stabilized).

/be

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 332866 [details] [diff] [review]
better fix

r=shaver.

Thanks for the missing OBJ_DROP_PROPERTY sprinkling, too.  If andreas' current type stability work doesn't cover this case well, we might want to look at increasing HOTLOOP to let us see things that are a little more stable?

Comment 9

[Brendan Eich [:brendan]]

Type stability is not the issue, rather shape stability. The JSOP_SETPROP code handles two cases:

1. Building an object by set, in which case it memoizes the pre-SETPROP shape and looks for a hit there before setting the next property and regenerating shape.

2. Resetting an already-defined property, which will hit on the pre-set shape but not regenerate (since the shape doesn't change).

By trace-recording on the second iteration, we miss the cached pre-set shape and abort recording. The interpreter misses too and refills with the now-stable, post-initial-set shape as key. Any iterations after this would hit in interpreter and recorder.

Shape inference speculates that the same pc ("call" site in the setter sense) will see the same shape next time. Both cases (1 and 2) are important. They can't be distinguished _a prior_. We just need a higher HOTLOOP1, which we're going to get (right?).

/be

Comment 10

[Andreas Gal :gal]

Is the recording aborted entirely? In that case we should try to record it again a few iterations later.

Comment 11

[Brendan Eich [:brendan]]

Grr, _a priori_. Fixed:

http://hg.mozilla.org/tracemonkey/index.cgi/rev/2960d21c2da4

Andreas: recording was aborted but this is exactly due to HOTLOOP == 2. Is there a way to abort but say "try again next iteration"?

/be

Comment 12

[Andreas Gal :gal]

You can try decrementing hits, so fragment->hits--. Its an ugly hack though.

Comment 13

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-449627.js,v  <--  regress-449627.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/f0e9fd501e63