Last Comment Bug 768364 - Shutdown crash [@ mozilla::hal_impl::ModifyWakeLock]
: Shutdown crash [@ mozilla::hal_impl::ModifyWakeLock]
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla16
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 326633 697132
  Show dependency treegraph
 
Reported: 2012-06-26 01:46 PDT by Jesse Ruderman
Modified: 2012-07-06 07:49 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (makes Firefox crash during shutdown) (219 bytes, text/html)
2012-06-26 01:46 PDT, Jesse Ruderman
no flags Details
stack trace (5.20 KB, text/plain)
2012-06-26 01:46 PDT, Jesse Ruderman
no flags Details
Clear sLockTable on shutdown (1.98 KB, patch)
2012-07-03 08:08 PDT, Kan-Ru Chen [:kanru] (UTC+8)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Clear sLockTable on shutdown (2.87 KB, patch)
2012-07-04 03:30 PDT, Kan-Ru Chen [:kanru] (UTC+8)
justin.lebar+bug: review+
Details | Diff | Splinter Review
Clear sLockTable on shutdown (2.88 KB, patch)
2012-07-05 19:08 PDT, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-26 01:46:01 PDT
Created attachment 636626 [details]
testcase (makes Firefox crash during shutdown)

1. Load the testcase
2. Quit Firefox

Result: Crash (bp-1a98674c-72b7-459b-915d-fd6b52120626)
Comment 1 Jesse Ruderman 2012-06-26 01:46:18 PDT
Created attachment 636627 [details]
stack trace
Comment 2 Justin Lebar (not reading bugmail) 2012-06-26 03:14:17 PDT
Kan-Ru, are you going to look at this?
Comment 3 Scoobidiver (away) 2012-06-26 03:17:14 PDT
On Windows: bp-84a87242-1517-4ab1-810c-e81e12120626.
Comment 4 Kan-Ru Chen [:kanru] (UTC+8) 2012-06-26 03:29:50 PDT
Looks like the lock table is cleared before the wakelock is GCed

  51  ClearOnShutdown(&sLockTable);
Comment 5 Justin Lebar (not reading bugmail) 2012-06-26 03:41:45 PDT
> 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.)
Comment 6 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-03 08:08:41 PDT
Created attachment 638737 [details] [diff] [review]
Clear sLockTable on shutdown
Comment 7 Justin Lebar (not reading bugmail) 2012-07-03 08:26:35 PDT
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!
Comment 8 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-04 02:36:28 PDT
(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?
Comment 9 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-04 03:30:52 PDT
Created attachment 639034 [details] [diff] [review]
Clear sLockTable on shutdown
Comment 10 Justin Lebar (not reading bugmail) 2012-07-05 07:46:55 PDT
> 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 11 Justin Lebar (not reading bugmail) 2012-07-05 07:50:17 PDT
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.
Comment 12 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-05 19:08:20 PDT
Created attachment 639555 [details] [diff] [review]
Clear sLockTable on shutdown

Address the nits.
Comment 13 Kan-Ru Chen [:kanru] (UTC+8) 2012-07-05 19:10:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/214bc2c2eeb2

Note You need to log in before you can comment on or make changes to this bug.