968956 - [DMD] crash in mozilla::dmd::ClearReports

Status: RESOLVED FIXED

(Core :: DMD, defect)

Description

[Eric Rahm [:erahm]]

Attachment: stack trace

I'm seeing sporadic crashes when dumping memory info on a DMD enabled build on my buri devices. The crash itself is in mozilla::dmd::ClearReports(), AFAICT this looks like a multi-threaded access of a hash table issue.

Comment 1

[Nicholas Nethercote [inactive]]

Good catch!

  // This lock must be held while manipulating global state, such as
  // gStackTraceTable, gBlockTable, etc.
  static Mutex* gStateLock = nullptr;

And it looks like ClearReports() does not get the lock.

Care to write a patch? It should be easy. Use the RAII AutoLockState class. And while you're doing that can you check that the lock is held for other uses of those global variables? Thanks!

Comment 2

[Eric Rahm [:erahm]]

Sure thing, I'll get a patch together and check the rest of the usages.

Comment 3

[Nicholas Nethercote [inactive]]

Bonus points if you create a wrapper class around js::HashSet that asserts if gStateLock is not held during accesses, and convert the global hash tables to use it :)

Comment 4

[Eric Rahm [:erahm]]

Attachment: Initial patch

Moved the core ClearReports logic to an unsafe (internal) function. The external interfaces now properly acquires the state lock. Properly checking that the current thread actually owns the mutex can probably be addressed in a separate bug.

Comment 5

[Eric Rahm [:erahm]]

Also of note: the DMD tests compiled, ran, and passed on my OS X machine. I also looked through for any other unlocked accesses to the has table and only found one case (which I don't think would have been an issue, but went ahead and added a lock anyhow).

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8372010 [details] [diff] [review]
Initial patch

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

Did you run the test?

Looks good. My only thought is if "Unsafe" is the best suffix; it's vague. Maybe ClearReportsNoLock()? I'll let you decide.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Comment on attachment 8372010 [details] [diff] [review]
> Initial patch
> 
> Review of attachment 8372010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you run the test?
> 
> Looks good. My only thought is if "Unsafe" is the best suffix; it's vague.
> Maybe ClearReportsNoLock()? I'll let you decide.

Tests were run (see Comment 5). Maybe just use| ClearReportsInternal| like SizeOf does?

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8372010 [details] [diff] [review]
Initial patch

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

Ok, ClearReportsInternal() sounds ok, since we have precedent.

::: memory/replace/dmd/DMD.cpp
@@ +2078,3 @@
>    if (!gIsDMDRunning) {
>      return;
>    }

Oh wait, this test should be in ClearReports(), not ClearReportsInternal().

Comment 9

[Eric Rahm [:erahm]]

Attachment: Renamed function

Switch from Unsafe to Internal and moved DMD running check to ClearReports

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8372631 [details] [diff] [review]
Renamed function

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

Since I gave r+ on the previous patch, you didn't need to ask again for review -- unless you really wanted it -- and you could have carried over the r+ yourself. But no matter.

I'll land this on Monday morning. Thanks.

Comment 11

[Nicholas Nethercote [inactive]]

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

Eric: I added "DONTBUILD because DMD is NPOTB." after the first line of the commit message. DMD (unfortunately) isn't tested on TBPL. The presence of "DONTBUILD" anywhere in the commit message tells TBPL not to run tests for this push. We do this for patches that are NPOTB ("not part of the build") in order to save machine time.

Comment 12

[Phil Ringnalda (:philor)]

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