Last Comment Bug 758408 - "Assertion failure: &obj->getSlotRef(slot) == this,"
: "Assertion failure: &obj->getSlotRef(slot) == this,"
Status: VERIFIED FIXED
[js:inv:p1][advisory-tracking+]
: assertion, regression, sec-critical, testcase, valgrind
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla17
Assigned To: Bill McCloskey (:billm)
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Mentors:
Depends on:
Blocks: jsfunfuzz 750834
  Show dependency treegraph
 
Reported: 2012-05-24 15:03 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-02-04 14:07 PST (History)
10 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
+
verified
+
verified
+
verified
15+
verified


Attachments
stack (including Valgrind stack) (23.56 KB, text/plain)
2012-05-24 15:03 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
patch (1.40 KB, patch)
2012-07-05 16:20 PDT, Bill McCloskey (:billm)
jwalden+bmo: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-05-24 15:03:36 PDT
Created attachment 626983 [details]
stack (including Valgrind stack)

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
Comment 1 Brian Hackett (:bhackett) 2012-05-24 16:14:07 PDT
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?
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2012-06-28 13:09:13 PDT
sec-critical based on invalid reads from Valgrind.
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2012-06-28 13:17:00 PDT
WFM on m-c changeset bf8f2961d0cc
Comment 4 Gary Kwong [:gkw] [:nth10sd] 2012-06-28 13:53:58 PDT
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.
Comment 5 Gary Kwong [:gkw] [:nth10sd] 2012-06-28 13:54:49 PDT
I could reproduce on 64-bit Mac js shells.
Comment 6 Gary Kwong [:gkw] [:nth10sd] 2012-06-28 13:55:28 PDT
Tested on m-c rev bf8f2961d0cc.
Comment 7 Bill McCloskey (:billm) 2012-07-05 13:41:08 PDT
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.
Comment 8 Bill McCloskey (:billm) 2012-07-05 16:20:11 PDT
Created attachment 639505 [details] [diff] [review]
patch

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.
Comment 9 Gary Kwong [:gkw] [:nth10sd] 2012-07-19 13:12:10 PDT
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 11 Bill McCloskey (:billm) 2012-07-19 17:03:48 PDT
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.
Comment 12 Gary Kwong [:gkw] [:nth10sd] 2012-07-19 17:32:21 PDT
Testcase to be landed when mozilla-beta makes it out to release in 6 weeks' time, setting in-testsuite? first.
Comment 13 Ed Morley [:emorley] 2012-07-20 06:54:27 PDT
https://hg.mozilla.org/mozilla-central/rev/2fe4ea4a614e
Comment 14 Alex Keybl [:akeybl] 2012-07-23 12:56:47 PDT
If bug 650369 is the regressing bug, this affects the ESR as well.
Comment 15 Alex Keybl [:akeybl] 2012-07-23 12:57:09 PDT
Comment on attachment 639505 [details] [diff] [review]
patch

[Triage Comment]
Low risk sg:crit fix, approving for all branches.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:03:38 PDT
Assigning to myself to resolve in-testsuite? flag to serve as verification. Gary, would you care to take some time to mentor me?
Comment 18 Gary Kwong [:gkw] [:nth10sd] 2012-08-14 18:55:02 PDT
(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.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-16 15:58:00 PDT
(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
Comment 20 Gary Kwong [:gkw] [:nth10sd] 2012-08-16 16:09:51 PDT
(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.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-24 16:03:12 PDT
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
Comment 22 Christian Holler (:decoder) 2013-02-04 14:07:15 PST
E4X has been removed, so we won't add the test.

Note You need to log in before you can comment on or make changes to this bug.