1438947 - Crash in mozilla::net::TRR::Cancel

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect, P1)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-d894c6f9-e5ef-4bb2-b529-c90f60180216.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::net::TRR::Cancel netwerk/dns/TRR.cpp:1036
1 xul.dll nsHostRecord::Cancel netwerk/dns/nsHostResolver.cpp:230
2 xul.dll nsHostResolver::ClearPendingQueue netwerk/dns/nsHostResolver.cpp:599
3 xul.dll nsHostResolver::Shutdown netwerk/dns/nsHostResolver.cpp:684
4 xul.dll nsDNSService::Shutdown netwerk/dns/nsDNSService2.cpp:676
5 xul.dll mozilla::net::nsIOService::SetOffline netwerk/base/nsIOService.cpp:1173
6 xul.dll mozilla::net::nsIOService::Observe netwerk/base/nsIOService.cpp:1488
7 xul.dll nsObserverList::NotifyObservers xpcom/ds/nsObserverList.cpp:112
8 xul.dll nsObserverService::NotifyObservers xpcom/ds/nsObserverService.cpp:296
9 xul.dll nsXREDirProvider::DoShutdown toolkit/xre/nsXREDirProvider.cpp:1095

=============================================================

There is 1 crash in nightly 60 with buildid 20180216104033. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1434852.

[1] https://hg.mozilla.org/mozilla-central/rev?node=6776d69d2f03

Comment 1

[Daniel Stenberg [:bagder]]

Clearly TRR related.

Comment 2

[Daniel Stenberg [:bagder]]

Attachment: bug 1438947 - avoid unnecessary timer cancels + clears

Review commit: https://reviewboard.mozilla.org/r/221054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221054/

Comment 3

[Valentin Gosu [:valentin] (he/him)]

mTimeout is of type nsCOMPtr<nsITimer>
So why do we have to initialize it to nullptr? I would expect the default constructor to do that already.

Comment 4

[Daniel Stenberg [:bagder]]

Ehm, right. I just checked TRR.cpp:1036 and figured it looked exactly like an uninitialized mTimeout in there... So its something else!

Comment 5

[Valentin Gosu [:valentin] (he/him)]

It seems to me that it might already be nullptr, and maybe there's a race with it being null-ed on another thread.

Comment 6

[Daniel Stenberg [:bagder]]

That might be it, yes. I'll go the other way instead and avoid some of the early cancel+clears since they're done in the destructor anyway. Another take is coming.

Comment 7

[Daniel Stenberg [:bagder]]

Hm, I bet it isn't even necessary to Cancel it in the destructor!

Comment 8

[Valentin Gosu [:valentin] (he/him)]

(In reply to Daniel Stenberg [:bagder] from comment #7)
> Hm, I bet it isn't even necessary to Cancel it in the destructor!

No. The destructor for nsTimer does that anyway.

Comment 9

[Daniel Stenberg [:bagder]]

Comment on attachment 8951786 [details]
bug 1438947 - avoid unnecessary timer cancels + clears

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221054/diff/1-2/

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8951786 [details]
bug 1438947 - avoid unnecessary timer cancels + clears

https://reviewboard.mozilla.org/r/221054/#review226968

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09cda613b1c4
avoid unnecessary timer cancels + clears r=valentin

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/09cda613b1c4

Comment 13

[Calixte Denizet (:calixte)]

There still are 12 crashes in nightly 60 starting with buildid 20180217100053 so after the patch landed.

Comment 14

[Daniel Stenberg [:bagder]]

In TRR::Cancel?

Comment 15

[Calixte Denizet (:calixte)]

Yes, see http://bit.ly/2HuBoKS to get the crash reports.

Comment 16

[Daniel Stenberg [:bagder]]

The crash reports indicate that there are at least two or three different ways that lead into the TRR::Cancel crash.

1 - (example https://crash-stats.mozilla.com/report/index/4d0fc238-85ed-444f-99a2-601e90180219) Via nsHostResolver::FlushCache that gets called from nsDNSService::Observe() from mozilla::net::nsIOService::NotifyWakeup. This is a machine waking up and it gets a network-change event and flushes the DNS cache.

2 - (example https://crash-stats.mozilla.com/report/index/5a160053-88e8-4416-80ee-de0890180219) Via nsHostRecord::Cancel() and nsHostResolver::Shutdown(). This is the browser shutting down.

3 - A variation of the first one (example https://crash-stats.mozilla.com/report/index/7e58d309-5468-4704-a008-f21760180219) Via nsHostRecord::Cancel and mozilla::net::nsIOService::SetOffline. This is the browser going offline and as a part of that, it flushes the DNS cache.

I can't see how any of them can be reached without TRR being activated, which is good since TRR is still preffed off by default.

Comment 17

[Daniel Stenberg [:bagder]]

I believe this might be a race when accessing/changing the mTrrA and mTrrAAAA in the nsHostRecord objects.

Partly this happens because the code currently cancels TRR resolves from nsHostRecord::RemoveOrRefresh, which I don't think it should since we don't need to refresh TRR name resolves due to network changes.

I'm testing out a fix for this locally which I'll submit for review as soon as I feel confident.

Comment 18

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1438947-mutex-nsHostRecord-mTrrA-AAA-accesses-r-.patch

(reviewboard refuses me to push another patch to the same bug that already had a patch previously so I'm doing it the old style as a work-around)

This patch puts a mutex around all nsHostRecord::mTrrA[AAA] accesses since they might be accessed and updated from different threads.
    
... and remove Cancel() from nsHostRecord::RemoveOrRefresh since we don't need to cancel TRR resolves due to network changes.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8952965 [details] [diff] [review]
0001-bug-1438947-mutex-nsHostRecord-mTrrA-AAA-accesses-r-.patch

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

::: netwerk/dns/nsHostResolver.cpp
@@ +1076,5 @@
>      RefPtr<nsHostRecord> rec(aRec);
>      mLock.AssertCurrentThreadOwns();
> +    {
> +        MutexAutoLock trrlock(rec->mTrr_lock);
> +        MOZ_ASSERT(!TRROutstanding());

Let's wrap this block in a #ifdef DEBUG, since we're only locking for this assert

::: netwerk/dns/nsHostResolver.h
@@ +189,5 @@
>      enum {
>          INIT, STARTED, OK, FAILED
>      } mTrrAUsed, mTrrAAAAUsed;
>  
> +    Mutex mTrr_lock; // lock when accessing the mTrrA[AAA] pointers

Let's call it mTrrLock or mTRRLock.

Comment 20

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1438947-mutex-nsHostRecord-mTrrA-AAA-accesses.patch

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11edb4e3cdec
mutex nsHostRecord::mTrrA[AAA] accesses r=valentin

Comment 22

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/11edb4e3cdec