832609 - test_823965.html : Assertion failure: (next == this) == (prev == this), at ../../../dist/include/mozilla/LinkedList.h:191

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Jonathan Kew (:jfkthame)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=18943119&tree=Mozilla-Inbound

Assertion failure: (next == this) == (prev == this), at ../../../dist/include/mozilla/LinkedList.h:191
TEST-UNEXPECTED-FAIL | /tests/dom/devicestorage/test/test_823965.html | Exited with code 11 during test run
PROCESS-CRASH | /tests/dom/devicestorage/test/test_823965.html | application crashed [@ mozilla::LinkedListElement<nsDOMMemoryFile::DataOwner>::isInList() const]
Thread 31 (crashed)

Operating system: Linux
                  0.0.0 Linux 2.6.31.5-127.fc12.x86_64 #1 SMP Sat Nov 7 21:11:14 EST 2009 x86_64
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  SIGSEGV
Crash address: 0x0

Thread 31 (crashed)
 0  libxul.so!mozilla::LinkedListElement<nsDOMMemoryFile::DataOwner>::isInList() const [LinkedList.h : 191 + 0x18]
    rbx = 0x000000000b07f720   r12 = 0x0000000000000000
    r13 = 0x0000000000000001   r14 = 0x00007f1d6aafecc0
    r15 = 0x0000000000000000   rip = 0x00007f1dccbd2f9e
    rsp = 0x00007f1d6aafeb10   rbp = 0x00007f1d6aafeb10
    Found by: given as instruction pointer in context
 1  libxul.so!mozilla::LinkedListElement<nsDOMMemoryFile::DataOwner>::~LinkedListElement [LinkedList.h : 120 + 0x4]
    rbx = 0x000000000b07f720   r12 = 0x0000000000000000
    r13 = 0x0000000000000001   r14 = 0x00007f1d6aafecc0
    r15 = 0x0000000000000000   rip = 0x00007f1dccbd3039
    rsp = 0x00007f1d6aafeb20   rbp = 0x00007f1d6aafeb30
    Found by: call frame info
 2  libxul.so!nsDOMMemoryFile::DataOwner::Release() [nsDOMFile.h : 399 + 0x7]
    rbx = 0x000000000b07f720   r12 = 0x0000000000000000
    r13 = 0x0000000000000001   r14 = 0x00007f1d6aafecc0
    r15 = 0x0000000000000000   rip = 0x00007f1dccbee8f9
    rsp = 0x00007f1d6aafeb40   rbp = 0x00007f1d6aafeb50
    Found by: call frame info
 3  libxul.so!DataOwnerAdapter::Release() [nsDOMFile.cpp : 87 + 0x4]
    rbx = 0x0000000002c04be0   r12 = 0x0000000000000000
    r13 = 0x0000000000000001   r14 = 0x00007f1d6aafecc0
    r15 = 0x0000000000000000   rip = 0x00007f1dccbee9d8
    rsp = 0x00007f1d6aafeb60   rbp = 0x00007f1d6aafeb70
    Found by: call frame info
 4  libxul.so!nsTArray_Impl<nsCOMPtr<nsIInputStream>, nsTArrayInfallibleAllocator>::DestructRange(unsigned int, unsigned int) [nsTArray.h : 432 + 0x4]
    rbx = 0x0000000003244ab0   r12 = 0x0000000003244ab0
    r13 = 0x0000000000000001   r14 = 0x00007f1d6aafecc0
    r15 = 0x0000000000000000   rip = 0x00007f1dcd843657
    rsp = 0x00007f1d6aafeb80   rbp = 0x00007f1d6aafeb90
    Found by: call frame info
 5  libxul.so!nsTArray_Impl<nsCOMPtr<nsIInputStream>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) [nsTArray.h : 1017 + 0xc]
    rbx = 0x0000000000000000   r12 = 0x0000000003e682b8
    r13 = 0x0000000000000001   r14 = 0x00007f1d6aafecc0
    r15 = 0x0000000000000000   rip = 0x00007f1dcd8438e3
    rsp = 0x00007f1d6aafeba0   rbp = 0x00007f1d6aafebc0
    Found by: call frame info
...etc.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can this LinkedList be used by multiple threads?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, it definitely can be ...

Comment 3

[Justin Lebar (not reading bugmail)]

...without locking?

What's happening here is that the linked list destructor is running, and it's checking whether the node is in a list.  In this case, it's partially in a list (one of the pointer is to self, but the other pointer is not).  That's only a valid state if the list is currently being mutated.  Which means that either we have memory corruption or the list is being mutated /while the destructor is running/.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well DataOwner's dtor can run on any thread, so if there's no locking we need to add some.

Comment 5

[Legacy TBPL/Treeherder Robot]

RyanVM
https://tbpl.mozilla.org/php/getParsedLog.php?id=19053383&tree=Mozilla-Inbound
Rev3 Fedora 12x64 mozilla-inbound debug test mochitest-2 on 2013-01-23 07:57:46
slave: talos-r3-fed64-025

Assertion failure: (next == this) == (prev == this), at ../../../dist/include/mozilla/LinkedList.h:191
TEST-UNEXPECTED-FAIL | /tests/dom/devicestorage/test/test_823965.html | Exited with code 11 during test run
PROCESS-CRASH | /tests/dom/devicestorage/test/test_823965.html | application crashed [@ mozilla::LinkedListElement<nsDOMMemoryFile::DataOwner>::isInList() const]

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 7

[Justin Lebar (not reading bugmail)]

khuey and I talked about this on IRC, and we agree this patch is scary.

So what we're going to do is disable this memory reporter on b2g18 (that's safest possible change we could make there) and then try a simpler approach for trunk (register one memory reporter for each large blob).  This simpler approach requires bug 833913.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Disable the reporter on b2g18.

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 705531 [details] [diff] [review]
Disable the reporter on b2g18.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 803684
User impact if declined: crashes, memory corruption, bad times
Testing completed: Looks about right to me.
Risk to taking this patch (and alternatives if risky): Hard to imagine things getting worse as a result of this patch.
String or UUID changes made by this patch: none

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3f59ebb61ca9

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 705531 [details] [diff] [review]
Disable the reporter on b2g18.

[Approval Request Comment]
See comment 9.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b7a0f44689c
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4f89f5b0d0f

Comment 13

[Legacy TBPL/Treeherder Robot]

anton
https://tbpl.mozilla.org/php/getParsedLog.php?id=19501288&tree=Fx-Team
Rev4 MacOSX Lion 10.7 fx-team opt test mochitest-2 on 2013-02-06 11:13:43
slave: talos-r4-lion-084

TEST-UNEXPECTED-FAIL | /tests/dom/devicestorage/test/test_823965.html | Exited with code 1 during test run
PROCESS-CRASH | /tests/dom/devicestorage/test/test_823965.html | application crashed [@ 0x0]

Comment 14

[Legacy TBPL/Treeherder Robot]

edmorley
https://tbpl.mozilla.org/php/getParsedLog.php?id=20774150&tree=Mozilla-Inbound
Rev3 WINNT 5.1 mozilla-inbound debug test mochitest-2 on 2013-03-18 04:06:27
slave: talos-r3-xp-083

04:15:18     INFO -  1971 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_oop_ScrollEvent.html | This test left crash dumps behind, but we weren't expecting it to!
04:16:20     INFO -  2 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/devicestorage/test/test_823965.html | This test left crash dumps behind, but we weren't expecting it to!
04:32:24     INFO -  9639 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/ipc/test_ipc.html | /tests/dom/indexedDB/test/test_add_put.html |  This test left crash dumps behind, but we weren't expecting it to!
04:45:57     INFO -  26819 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudioVideoCombined.html | Assertion count 15 is greater than expected range 0-13 assertions.
04:46:00     INFO -  26855 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_basicVideo.html | Assertion count 172 is greater than expected range 0-0 assertions.
04:46:01     INFO -  26859 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_bug822674.html | Assertion count 39 is greater than expected range 0-0 assertions.
04:46:07     INFO -  26946 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_bug835370.html | Assertion count 101 is greater than expected range 0-9 assertions.
04:46:09     INFO -  26983 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_bug840344.html | Assertion count 126 is greater than expected range 0-125 assertions.
04:46:41     INFO -  PROCESS-CRASH | Main app process exited normally | application crashed [@ ntdll.dll + 0x1230]
04:46:45    ERROR - Return code: 1

Comment 15

[Justin Lebar (not reading bugmail)]

Aand we never followed up with this bug.  :(

Comment 17

[Justin Lebar (not reading bugmail)]

The paper-over fix needs to be uplifted to Aurora again, and we need to actually fix this.  It's not just the dtor; the constructor also can run on any thread, afaict from bug 873623.

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 705531 [details] [diff] [review]
Disable the reporter on b2g18.

We need to re-land this on Aurora.  :(

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah crap.  I hate it when I lose track of bugs.

Comment 20

[Justin Lebar (not reading bugmail)]

I'm compiling a patch for this that involves making a StaticMutex, which is something I've wanted in many other cases.  If this works, it would be pretty nice...

Comment 21

[Justin Lebar (not reading bugmail)]

No way this will compile anywhere else.

https://tbpl.mozilla.org/?tree=Try&rev=6469459a5db5

Comment 22

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=c7d94b3376ac

Comment 23

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=807ae33a65b1

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1: Add proper locking around nsDOMFile's memory reporter infrastructure.

Depends on bug 873801, which has its own dependency.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 705531 [details] [diff] [review]
Disable the reporter on b2g18.

Approving for Aurora 23 - should we also get this on Beta 22?  We aren't tracking this bug because it's not clear that we have signs of significant, reproducible user impact here.  Please nominate if you believe it to be more serious.

Comment 26

[Justin Lebar (not reading bugmail)]

> We aren't tracking this bug because it's not clear that we have signs of significant, 
> reproducible user impact here.

Probably because few sites use workers.

> should we also get this on Beta 22?

Yes; thanks for asking.

> Please nominate if you believe it to be more serious.

I believe this to be pretty serious.  I'll elaborate in a PM.

Comment 27

[Justin Lebar (not reading bugmail)]

Comment on attachment 705531 [details] [diff] [review]
Disable the reporter on b2g18.

Re-nom'ing for beta.  It's a shame we missed this for the last release, but that ship has sailed.

Comment 28

[Justin Lebar (not reading bugmail)]

Checkin-needed to re-land on Aurora (and beta, if and when we get re-approval).

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/729fb1f47da8

Not sure what the beta holdup is here...

Comment 30

[Alex Keybl [:akeybl]]

(In reply to Ryan VanderMeulen [:RyanVM][Out May 23-27] from comment #29)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/729fb1f47da8
> 
> Not sure what the beta holdup is here...

It's not clear that this intermittent failure is hitting FF22, so we were waiting for jlebar to elaborate on severity here (didn't get a PM).

Comment 31

[Justin Lebar (not reading bugmail)]

I sent a PM to lsblakk immediately after posting the comment.  I didn't get a response; maybe it didn't go through.

I'll just say it here: I think you could exploit this bug without a lot of difficulty.

Comment 32

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0ac9d38a1b

checkin-needed for mozilla-beta still stands.

Comment 33

[Justin Lebar (not reading bugmail)]

Gah, https://hg.mozilla.org/integration/mozilla-inbound/rev/56f89cd46261 landed with this bug in the commit message instead of bug 873801.  At least they're related bugs...

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/56f89cd46261
https://hg.mozilla.org/mozilla-central/rev/8d0ac9d38a1b

Comment 35

[Justin Lebar (not reading bugmail)]

Ah, that's why this hasn't landed on beta: The m-c merge removed checkin-needed.

I'll land this on beta myself.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

That and the whole holiday weekend thing...

Comment 37

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-beta/rev/48a67a635157

Comment 38

[David Teller [:Yoric] - still alive but not very active]

I just hit the following assertion: http://pastebin.mozilla.org/2565047
It was my understanding that this bug should have fixed it.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's not an assertion, that's just a stack.  I bet that's a refcounting threadsafety assertion firing.

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Well, now it's an assertion + a stack:)
http://pastebin.mozilla.org/2565061

And yes, it looks like a refcounting threadsafety assertion.

Comment 41

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah my guess is that we create it on whatever thread wants to use it first which may not be the main thread.  It should really just have threadsafe refcounting.

Open a new bug please.