The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: kanru)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla16
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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)
(Reporter)

Comment 1

5 years ago
Created attachment 636627 [details]
stack trace
Kan-Ru, are you going to look at this?

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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.)
(Assignee)

Comment 6

5 years ago
Created attachment 638737 [details] [diff] [review]
Clear sLockTable on shutdown
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-
(Assignee)

Comment 8

5 years ago
(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?
(Assignee)

Comment 9

5 years ago
Created attachment 639034 [details] [diff] [review]
Clear sLockTable on shutdown
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+
(Assignee)

Comment 12

5 years ago
Created attachment 639555 [details] [diff] [review]
Clear sLockTable on shutdown

Address the nits.
Attachment #639034 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/214bc2c2eeb2
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/214bc2c2eeb2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.