Closed Bug 449627 Opened 11 years ago Closed 11 years ago

TM: crash in js_FillPropertyCache

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sayrer, Assigned: brendan)

References

Details

(Keywords: testcase)

Attachments

(4 files, 1 obsolete file)

Crash in-browser on the UA test used on mozilla.com
Attached file test case
the detection code that triggers the crash
Blocks: landtm
Same stack on gmail.com, after logging in.
Attached file shell test case
Attached patch fix (obsolete) — Splinter Review
Rob, could you please test this? I've got other stuff in flight and could use the assist. Thanks,

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #332815 - Flags: review?(shaver)
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.
Attached patch better fixSplinter Review
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
Attachment #332815 - Attachment is obsolete: true
Attachment #332866 - Flags: review?(shaver)
Attachment #332815 - Flags: review?(shaver)
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?
Attachment #332866 - Flags: review?(shaver) → review+
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
Is the recording aborted entirely? In that case we should try to record it again a few iterations later.
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You can try decrementing hits, so fragment->hits--. Its an ugly hack though.
Keywords: testcase
/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
Flags: in-testsuite+
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.