Closed Bug 758408 Opened 8 years ago Closed 8 years ago

"Assertion failure: &obj->getSlotRef(slot) == this,"

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox13 --- unaffected
firefox14 --- unaffected
firefox15 + verified
firefox16 + verified
firefox17 + verified
firefox-esr10 15+ verified

People

(Reporter: gkw, Assigned: billm)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [js:inv:p1][advisory-tracking+])

Attachments

(2 files)

function f(y) {
  x = ["", "", "", "", "", "", "", "", "", "", "", "", "", ""]
  for (var i = 0; i < x.length; ++i) {
    y[i] = []
  }
  return y
}
evalcx("undefined.function::e", f(evalcx("lazy")))

asserts js debug shell on m-c changeset 6ad95e65d74d without any CLI arguments at Assertion failure: &obj->getSlotRef(slot) == this,

s-s because Valgrind shows invalid reads in an opt build.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   93639:fbff86190de6
user:        Brian Hackett
date:        Wed May 09 14:14:10 2012 -0700
summary:     Don't throw away JIT code for compartments in web pages displaying animations, bug 750834. r=billm
I doubt fbff86190de6 is involved (though it's not impossible), as the method JIT isn't enabled here and the GC related changes in fbff86190de6 are mainly gated in the shell on gcPreserveCode() being called, which isn't the case here.  This looks like a issue with barriers and e4x?
Whiteboard: js-triage-needed → [js:inv:p1]
sec-critical based on invalid reads from Valgrind.
Keywords: sec-critical
WFM on m-c changeset bf8f2961d0cc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Actually, not true:

options('allow_xml');
function f(y) {
  x = ["", "", "", "", "", "", "", "", "", "", "", "", "", ""]
  for (var i = 0; i < x.length; ++i) {
    y[i] = []
  }
  return y
}
evalcx("undefined.function::e", f(evalcx("lazy")));


still asserts identically.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
I could reproduce on 64-bit Mac js shells.
Status: REOPENED → NEW
OS: Linux → All
Hardware: x86 → All
Tested on m-c rev bf8f2961d0cc.
I'll take this, but I doubt it's related to incremental GC. The testcase isn't running any incremental GCs or any barrier verification, so the write barriers shouldn't be triggering.
Assignee: general → wmccloskey
Attached patch patchSplinter Review
We call getSlotRef. Later we call NewXMLNamespace, which causes the slots vector to be reallocated at a different place. Finally we try to use the result of getSlotRef again. It asserts the location it returned is no longer the right one.

I think this regressed in bug 650369, which landed in FF6.
Attachment #639505 - Flags: review?(jwalden+bmo)
Attachment #639505 - Flags: review?(jwalden+bmo) → review+
Bill, I wanted to help check this in but it bitrotted a little.

$ hg qpush
applying bug758408.patch
patching file js/src/jsxml.cpp
Hunk #1 FAILED at 7495
Hunk #2 FAILED at 7514
2 out of 2 hunks FAILED -- saving rejects to file js/src/jsxml.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug758408.patch
Comment on attachment 639505 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 650369
User impact if declined: Possible crashes, exploitation.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None.
Attachment #639505 - Flags: approval-mozilla-esr10?
Attachment #639505 - Flags: approval-mozilla-beta?
Attachment #639505 - Flags: approval-mozilla-aurora?
Testcase to be landed when mozilla-beta makes it out to release in 6 weeks' time, setting in-testsuite? first.
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/2fe4ea4a614e
Status: NEW → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
If bug 650369 is the regressing bug, this affects the ESR as well.
Comment on attachment 639505 [details] [diff] [review]
patch

[Triage Comment]
Low risk sg:crit fix, approving for all branches.
Attachment #639505 - Flags: approval-mozilla-esr10?
Attachment #639505 - Flags: approval-mozilla-esr10+
Attachment #639505 - Flags: approval-mozilla-beta?
Attachment #639505 - Flags: approval-mozilla-beta+
Attachment #639505 - Flags: approval-mozilla-aurora?
Attachment #639505 - Flags: approval-mozilla-aurora+
Whiteboard: [js:inv:p1] → [js:inv:p1][advisory-tracking+]
Assigning to myself to resolve in-testsuite? flag to serve as verification. Gary, would you care to take some time to mentor me?
QA Contact: anthony.s.hughes
Whiteboard: [js:inv:p1][advisory-tracking+] → [js:inv:p1][advisory-tracking+][qa+]
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #17)
> Assigning to myself to resolve in-testsuite? flag to serve as verification.
> Gary, would you care to take some time to mentor me?

No problem - try saving comment 4 into a file (758408-c4.js for example and run ./js 758408-c4.js where the js binary corresponds to the binaries according to the changesets below):

Do you see this assertion on mozilla-central changeset 2fe4ea4a614e ?

Do you see this assertion on its parent, mozilla-central changeset b56cc7740240 ?

If the first one is no, and the second one is yes, then you're successfully verified that mozilla-central changeset 2fe4ea4a614e would have fixed this.
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #18)
> Do you see this assertion on mozilla-central changeset 2fe4ea4a614e ?

I see the following:
$ ./objdir-ff-debug/js/src/js ../758408-c4.js 
../758408-c4.js:9: TypeError: undefined has no properties
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #19)
> (In reply to Gary Kwong [:gkw, :nth10sd] from comment #18)
> > Do you see this assertion on mozilla-central changeset 2fe4ea4a614e ?
> 
> I see the following:
> $ ./objdir-ff-debug/js/src/js ../758408-c4.js 
> ../758408-c4.js:9: TypeError: undefined has no properties

This likely means we need a comment at the top of the .js test:

// |jit-test| TypeError

to show that the TypeError is expected.
Confirmed testcase reproducible with 2012-05-24 mozilla-central debug JS shell.

Verified fixed with:
 * 2012-08-24 mozilla-central debug JS shell
 * 2012-08-24 mozilla-aurora debug JS shell
 * 2012-08-24 mozilla-beta debug JS shell
 * 2012-08-24 mozilla-esr10 debug JS shell
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [js:inv:p1][advisory-tracking+][qa+] → [js:inv:p1][advisory-tracking+]
Group: core-security
E4X has been removed, so we won't add the test.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.