bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

TM: crash in js_FillPropertyCache

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Robert Sayre, Assigned: brendan)

Tracking

({testcase})

unspecified
x86
Mac OS X
testcase
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Crash in-browser on the UA test used on mozilla.com
(Reporter)

Comment 1

10 years ago
Created attachment 332768 [details]
backtraces for native stack, js stack
(Reporter)

Comment 2

10 years ago
Created attachment 332769 [details]
test case

the detection code that triggers the crash
(Reporter)

Updated

10 years ago
Blocks: 449436
(Reporter)

Comment 3

10 years ago
Same stack on gmail.com, after logging in.
(Reporter)

Comment 4

10 years ago
Created attachment 332808 [details]
shell test case
(Assignee)

Comment 5

10 years ago
Created attachment 332815 [details] [diff] [review]
fix

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)
(Reporter)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
Created attachment 332866 [details] [diff] [review]
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
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+
(Assignee)

Comment 9

10 years ago
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

10 years ago
Is the recording aborted entirely? In that case we should try to record it again a few iterations later.
(Assignee)

Comment 11

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 12

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

Updated

10 years ago
Keywords: testcase

Comment 13

10 years ago
/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.