824484 - Intermittent crash in test_privatebrowsing_cancel.js, test_bug430120.js [@ntdll.dll + 0x2fc47], with nsCacheEntryDescriptor::nsOutputStreamWrapper::Close() on the stack

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Attachment: crash stack

First seen on inbound: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=372d779d8f27&jobname=Rev3%20WINNT%206.1%20mozilla-inbound%20opt%20test%20xpcshell

https://tbpl.mozilla.org/php/getParsedLog.php?id=18235934&tree=Mozilla-Inbound&full=1#error0

See attached stack.

Comment 1

[Legacy TBPL/Treeherder Robot]

RyanVM
https://tbpl.mozilla.org/php/getParsedLog.php?id=18246020&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound opt test xpcshell on 2012-12-24 11:51:41
slave: talos-r3-w7-103

TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 2

[Legacy TBPL/Treeherder Robot]

gps
https://tbpl.mozilla.org/php/getParsedLog.php?id=18259874&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound opt test xpcshell on 2012-12-25 12:19:50
slave: talos-r3-w7-005

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | c:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 3

[Legacy TBPL/Treeherder Robot]

RyanVM
https://tbpl.mozilla.org/php/getParsedLog.php?id=18270350&tree=Firefox
Rev3 WINNT 6.1 mozilla-central opt test xpcshell on 2012-12-26 05:57:09
slave: talos-r3-w7-011

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\extensions\test\xpcshell-unpack\test_bug430120.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\extensions\test\xpcshell-unpack\test_bug430120.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 4

[Legacy TBPL/Treeherder Robot]

philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=18264477&tree=Profiling
Rev3 WINNT 6.1 profiling opt test xpcshell on 2012-12-25 23:11:03
slave: talos-r3-w7-051

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\extensions\test\xpcshell-unpack\test_bug430120.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | c:\talos-slave\test\build\xpcshell\tests\toolkit\mozapps\extensions\test\xpcshell-unpack\test_bug430120.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 5

[Legacy TBPL/Treeherder Robot]

philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=18273930&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound pgo test xpcshell on 2012-12-26 09:40:39
slave: talos-r3-w7-011

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | c:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 6

[Legacy TBPL/Treeherder Robot]

philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=18304591&tree=Firefox
Rev3 WINNT 6.1 mozilla-central pgo test xpcshell on 2012-12-27 14:56:18
slave: talos-r3-w7-077

TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 7

[Legacy TBPL/Treeherder Robot]

ehsan
https://tbpl.mozilla.org/php/getParsedLog.php?id=18345164&tree=Firefox
Rev3 WINNT 6.1 mozilla-central pgo test xpcshell on 2012-12-29 15:46:23
slave: talos-r3-w7-054

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | c:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 8

[Legacy TBPL/Treeherder Robot]

graememcc_firefox%graeme-online.co.uk
https://tbpl.mozilla.org/php/getParsedLog.php?id=18361499&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound pgo test xpcshell on 2012-12-30 21:32:40
slave: talos-r3-w7-076

TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 9

[Legacy TBPL/Treeherder Robot]

philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=18372417&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound opt test xpcshell on 2012-12-31 10:23:16
slave: talos-r3-w7-074

TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 10

[Michal Novotny [:michal]]

Attachment: patch v1

Grabbing a reference to the stream under the cache lock in nsCacheService::CloseAllStreams() doesn't ensure that it won't go away since it might be already waiting for the lock in the destructor. This patch fixes the problem.

Comment 11

[Michal Novotny [:michal]]

Tryserver looks good.

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

Comment 12

[Michal Novotny [:michal]]

Attachment: patch v2

With the change from bug #808997, input and output streams can live longer than cache service. So we must not grab the cache lock when we are not sure that the cache service exists, i.e. when mDescriptor is null.

tryserver push: https://tbpl.mozilla.org/?tree=Try&rev=3664f349ba47

Comment 13

[Michal Novotny [:michal]]

Comment on attachment 697008 [details] [diff] [review]
patch v2

Brian seems to be busy, reassigning to Nick.

Comment 14

[u408661]

Comment on attachment 697008 [details] [diff] [review]
patch v2

Review of attachment 697008 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, just one minor nit below.

::: netwerk/cache/nsCacheEntryDescriptor.cpp
@@ +674,5 @@
> +    count = NS_AtomicDecrementRefcnt(mRefCnt);
> +    NS_LOG_RELEASE(this, count, "nsCacheEntryDescriptor::nsInputStreamWrapper");
> +
> +    if (0 == count) {
> +        if (mDescriptor) {

Is there a particular reason to use mDescriptor instead of desc here? Just seems odd to me to use the thing you've already gotten a local reference to. If there's a reason I'm missing, please add a comment explaining why you use mDescriptor instead of desc before landing. (Same goes for other Release functions in other places in this patch.)

Comment 15

[Legacy TBPL/Treeherder Robot]

philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=18414247&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound opt test xpcshell on 2013-01-02 15:55:27
slave: talos-r3-w7-037

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | c:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 16

[Legacy TBPL/Treeherder Robot]

edmorley
https://tbpl.mozilla.org/php/getParsedLog.php?id=18432652&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound opt test xpcshell on 2013-01-03 05:55:15
slave: talos-r3-w7-098

TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 17

[Legacy TBPL/Treeherder Robot]

edmorley
https://tbpl.mozilla.org/php/getParsedLog.php?id=18437337&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound opt test xpcshell on 2013-01-03 09:05:54
slave: talos-r3-w7-071

TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | test failed (with xpcshell return code: -1073741819), see following log:
PROCESS-CRASH | C:\talos-slave\test\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_privatebrowsing_cancel.js | application crashed [@ ntdll.dll + 0x2fc47]
Thread 0 (crashed)

Comment 18

[Michal Novotny [:michal]]

Attachment: patch v3 - added comment

(In reply to Nick Hurley [:hurley] from comment #14)
> Is there a particular reason to use mDescriptor instead of desc here? Just
> seems odd to me to use the thing you've already gotten a local reference to.
> If there's a reason I'm missing, please add a comment explaining why you use
> mDescriptor instead of desc before landing. (Same goes for other Release
> functions in other places in this patch.)

mDescriptor can be nulled out on another thread between releasing mLock and acquiring cache lock. We would need to grab the cache lock under the mLock but then we would release these locks in non-LIFO order which causes warning here:

http://hg.mozilla.org/mozilla-central/annotate/6955309291ee/xpcom/glue/BlockingResourceBase.cpp#l167

Even with the current locking, it wouldn't be a big problem to use desc instead of mDescriptor because output streams just null out back-reference. But in case of input streams the assertion would need to be removed since the wrapper might be already removed from the array. Anyway, the current code is more clean and correct.

Comment 19

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac40d537b30a

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ac40d537b30a