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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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 [Laid off!]]

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] (NOT official MoCo now)]

sec-critical based on invalid reads from Valgrind.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

WFM on m-c changeset bf8f2961d0cc

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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] (NOT official MoCo now)]

I could reproduce on 64-bit Mac js shells.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Tested on m-c rev bf8f2961d0cc.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 [inactive unless it's an emergency] (:billm)]

Attachment: 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] (NOT official MoCo now)]

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 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe4ea4a614e

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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] (NOT official MoCo now)]

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]]

https://hg.mozilla.org/mozilla-central/rev/2fe4ea4a614e

Comment 14

[Alex Keybl [:akeybl]]

If bug 650369 is the regressing bug, this affects the ESR as well.

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 639505 [details] [diff] [review]
patch

[Triage Comment]
Low risk sg:crit fix, approving for all branches.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/81c6465d31b6
https://hg.mozilla.org/releases/mozilla-beta/rev/5c6e3097670e
https://hg.mozilla.org/releases/mozilla-esr10/rev/fc115a7cc14a

Comment 17

[u279076]

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] (NOT official MoCo now)]

(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

[u279076]

(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] (NOT official MoCo now)]

(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

[u279076]

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)]

E4X has been removed, so we won't add the test.