Closed Bug 768364 Opened 7 years ago Closed 7 years ago

Shutdown crash [@ mozilla::hal_impl::ModifyWakeLock]

Categories

(Core :: DOM: Core & HTML, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: jruderman, Assigned: kanru)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files, 2 obsolete files)

1. Load the testcase
2. Quit Firefox

Result: Crash (bp-1a98674c-72b7-459b-915d-fd6b52120626)
Attached file stack trace
Kan-Ru, are you going to look at this?
On Windows: bp-84a87242-1517-4ab1-810c-e81e12120626.
Crash Signature: [@ mozilla::hal_impl::ModifyWakeLock] [@ PL_DHashTableOperate | mozilla::hal_impl::ModifyWakeLock ] → [@ mozilla::hal_impl::ModifyWakeLock] [@ PL_DHashTableOperate | mozilla::hal_impl::ModifyWakeLock ] [@ PL_DHashTableOperate | nsTHashtable<nsBaseHashtableET<nsPtrHashKey<PRThread>, nsRefPtr<nsThread> > >::GetEntry(PRThread*)]
OS: Mac OS X → All
Hardware: x86_64 → All
Looks like the lock table is cleared before the wakelock is GCed

  51  ClearOnShutdown(&sLockTable);
> Looks like the lock table is cleared before the wakelock is GCed

Sigh.  And I can't really make ClearOnShutdown happen after that GC/CC; we need to release the refs before the GC/CC so that the GC/CC deletes those objects.

Can you just register a shutdown observer and empty out the hashtable?  That should fix this.

(But I'm starting to believe that ClearOnShutdown is not the right API, in general.)
Attached patch Clear sLockTable on shutdown (obsolete) — Splinter Review
Assignee: nobody → kchen
Attachment #638737 - Flags: review?(justin.lebar+bug)
Comment on attachment 638737 [details] [diff] [review]
Clear sLockTable on shutdown

>+class ClearHashtableOnShutdown : public nsIObserver {
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIOBSERVER
>+};

Nit: Please put the class in an anonymous namespace.

>+NS_IMPL_ISUPPORTS1(ClearHashtableOnShutdown, nsIObserver)
>+
>+NS_IMETHODIMP
>+ClearHashtableOnShutdown::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* data)
>+{
>+  if (strcmp(aTopic, "xpcom-shutdown")) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+  obs->RemoveObserver(this, "xpcom-shutdown");

I don't think you need to do this.

>+  sLockTable = nsnull;

I wonder if we can trigger a call to ModifyWakeLock or GetWakeLockInfo during shutdown.  For example, nsNPAPIPluginInstance::~nsNPAPIPluginInstance() calls ModifyWakeLock -- can we guarantee that all plugins are destructed before xpcom-shutdown starts?  (We didn't have this problem with ClearOnShutdown(), in theory anyway, because that runs after xpcom-shutdown completes.)

It seems safer to me to set sLockTable to use !!sLockTable how you use currently use sInitialized.  That way, if someone happens to touch the lock table too late, we won't crash and burn.

> static void
> Init()
> {
>   sLockTable = new nsDataHashtable<nsStringHashKey, LockCount>();
>   sLockTable->Init();
>-  ClearOnShutdown(&sLockTable);
>+
>+  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+  obs->AddObserver(new ClearHashtableOnShutdown(), "xpcom-shutdown", false);

If we change things so that you can call Init() during or after shutdown, you need to check that the observer service is not null.

I'd like to have another look, although I'd be willing to re-review as-is if you can convince me that the code is safe as written!
Attachment #638737 - Flags: review?(justin.lebar+bug) → review-
(In reply to Justin Lebar [:jlebar] from comment #7)
> >+  sLockTable = nsnull;
> 
> I wonder if we can trigger a call to ModifyWakeLock or GetWakeLockInfo
> during shutdown.  For example,
> nsNPAPIPluginInstance::~nsNPAPIPluginInstance() calls ModifyWakeLock -- can
> we guarantee that all plugins are destructed before xpcom-shutdown starts? 
> (We didn't have this problem with ClearOnShutdown(), in theory anyway,
> because that runs after xpcom-shutdown completes.)

If ClearOnShutdown runs after xpcom-shutdown completes then we are clearing sLockTable earlier than that! It would have the same problem as the old code.

> It seems safer to me to set sLockTable to use !!sLockTable how you use
> currently use sInitialized.  That way, if someone happens to touch the lock
> table too late, we won't crash and burn.

Let's guard ModifyWakeLock with sIsShutingdown and do nothing in that case. Does it make sense?
Attached patch Clear sLockTable on shutdown (obsolete) — Splinter Review
Attachment #638737 - Attachment is obsolete: true
Attachment #639034 - Flags: review?(justin.lebar+bug)
> If ClearOnShutdown runs after xpcom-shutdown completes then we are clearing sLockTable earlier 
> than that!

I'm not sure what you mean; either we completely understand each other, or we completely misunderstand.  :)
Comment on attachment 639034 [details] [diff] [review]
Clear sLockTable on shutdown

>+NS_IMETHODIMP
>+ClearHashtableOnShutdown::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* data)
>+{
>+  if (!strcmp(aTopic, "xpcom-shutdown")) {

Nit: Please MOZ_ASSERT this instead of using an if statement.

>+  if (sIsShutingdown) {

Nit: sIsShuttingDown.
Attachment #639034 - Flags: review?(justin.lebar+bug) → review+
Address the nits.
Attachment #639034 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/214bc2c2eeb2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.