999341 - Shutdown crash in nsDocument::OnPageHide(bool, mozilla::dom::EventTarget*)

Status: VERIFIED FIXED

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

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-220efbfe-f8df-48c8-9fc0-d03df2140414.
=============================================================

#18 crash on 28 release. Also occurs on 29 to 31. Most stacks show this happening during shutdown. 

|os| is nullptr here:

  // Dispatch observer notification to notify observers page is hidden.
  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
  nsIPrincipal *principal = GetPrincipal();
  os->NotifyObservers(static_cast<nsIDocument*>(this),
                      nsContentUtils::IsSystemPrincipal(principal) ?
                        "chrome-page-hidden" :
                        "content-page-hidden",
                      nullptr);

Looks like this code came from:
Bug 843910 - Expose document state changes via observer service r=mrbkap 
Irakli, can you take a look?

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I'm afraid I have no idea how observer service may be a `nullptr` in some cases, but maybe Blake can give me some clues ?

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Only thing I can think of is to wrap the call into if (os) {...} but that kind of feels awkward.

Comment 3

[(Away)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #2)
> Only thing I can think of is to wrap the call into if (os) {...} but that
> kind of feels awkward.

It seems most callers of GetObserverService do that, or use an error macro. Some even have comments about null-ness during shutdown. I don't know whether that's the correct thing to do here.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to David Major [:dmajor] (UTC+12) from comment #3)
> It seems most callers of GetObserverService do that, or use an error macro.
> Some even have comments about null-ness during shutdown. I don't know
> whether that's the correct thing to do here.

I should have seen this during review: we definitely need to null-check the value of os here precisely because of shutdown. I wouldn't expect to have to do for the pageshow notification, though as that shouldn't happen so late in shutdown.

Comment 5

[(no longer active)]

I got this in a debug build:

16:12.22 [39933] ###!!! ABORT: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file /home/ehsan/mozilla-central-32/xpcom/build/../glue/nsCOMPtr.h, line 870
16:13.91 nsCOMPtr<nsIObserverService>::operator->() const (/home/ehsan/mozilla-central-32/xpcom/build/../glue/nsCOMPtr.h:870 (discriminator 1))
16:13.99 nsDocument::OnPageHide(bool, mozilla::dom::EventTarget*) (/home/ehsan/mozilla-central-32/content/base/src/nsDocument.cpp:8882)
16:14.02 nsDocumentViewer::PageHide(bool) (/home/ehsan/mozilla-central-32/layout/base/nsDocumentViewer.cpp:1321)
16:14.05 nsDocShell::FirePageHideNotification(bool) (/home/ehsan/mozilla-central-32/docshell/base/nsDocShell.cpp:1707)
16:14.05 nsDocShell::Destroy() (/home/ehsan/mozilla-central-32/docshell/base/nsDocShell.cpp:5271)
16:14.07 nsWebBrowser::SetDocShell(nsIDocShell*) (/home/ehsan/mozilla-central-32/embedding/browser/webBrowser/nsWebBrowser.cpp:1604)
16:14.07 nsWebBrowser::InternalDestroy() (/home/ehsan/mozilla-central-32/embedding/browser/webBrowser/nsWebBrowser.cpp:100)
16:14.07 ~nsCOMPtr (/home/ehsan/mozilla-central-32/obj-i686-pc-linux-gnu/embedding/browser/webBrowser/../../../dist/include/nsCOMPtr.h:922)
16:14.07 operator delete (/home/ehsan/mozilla-central-32/obj-i686-pc-linux-gnu/embedding/browser/webBrowser/../../../dist/include/mozilla/mozalloc.h:225)
16:14.07 nsWebBrowser::Release() (/home/ehsan/mozilla-central-32/embedding/browser/webBrowser/nsWebBrowser.cpp:129)
16:14.10 nsCOMPtr<nsIWebBrowser>::~nsCOMPtr() (/home/ehsan/mozilla-central-32/obj-i686-pc-linux-gnu/dom/ipc/../../dist/include/nsCOMPtr.h:532)
16:14.11 operator delete (/home/ehsan/mozilla-central-32/obj-i686-pc-linux-gnu/xpfe/appshell/src/../../../dist/include/mozilla/mozalloc.h:225)
16:14.13 ReleaseSliceNow(unsigned int, void*) (/home/ehsan/mozilla-central-32/xpcom/base/CycleCollectedJSRuntime.cpp:1008 (discriminator 1))
16:14.13 mozilla::IncrementalFinalizeRunnable::ReleaseNow(bool) (/home/ehsan/mozilla-central-32/xpcom/base/CycleCollectedJSRuntime.cpp:1081)
16:14.13 mozilla::CycleCollectedJSRuntime::FinalizeDeferredThings(mozilla::CycleCollectedJSRuntime::DeferredFinalizeType) (/home/ehsan/mozilla-central-32/xpcom/base/CycleCollectedJSRuntime.cpp:1131)
16:14.13 mozilla::CycleCollectedJSRuntime::OnGC(JSGCStatus) (/home/ehsan/mozilla-central-32/xpcom/base/CycleCollectedJSRuntime.cpp:1158)
16:14.25 Collect (/home/ehsan/mozilla-central-32/js/src/jsgc.cpp:4792 (discriminator 1))
16:14.25 JS::GCForReason(JSRuntime*, JS::gcreason::Reason) (/home/ehsan/mozilla-central-32/js/src/jsfriendapi.cpp:195)
16:14.26 mozilla::CycleCollectedJSRuntime::Collect(unsigned int) const (/home/ehsan/mozilla-central-32/xpcom/base/CycleCollectedJSRuntime.cpp:964)
16:14.26 nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) (/home/ehsan/mozilla-central-32/xpcom/base/nsCycleCollector.cpp:3445)
16:14.26 nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*) (/home/ehsan/mozilla-central-32/xpcom/base/nsCycleCollector.cpp:3307)
16:14.26 nsCycleCollector::ShutdownCollect() (/home/ehsan/mozilla-central-32/xpcom/base/nsCycleCollector.cpp:3264)
16:14.26 nsRefPtr<nsCycleCollector>::assign_with_AddRef(nsCycleCollector*) (/home/ehsan/mozilla-central-32/obj-i686-pc-linux-gnu/xpcom/base/../../dist/include/nsAutoPtr.h:866)
16:14.27 mozilla::ShutdownXPCOM(nsIServiceManager*) (/home/ehsan/mozilla-central-32/xpcom/build/nsXPComInit.cpp:885)
16:14.28 ScopedXPCOMStartup::~ScopedXPCOMStartup() (/home/ehsan/mozilla-central-32/toolkit/xre/nsAppRunner.cpp:1203 (discriminator 1))
16:14.28 operator delete (/home/ehsan/mozilla-central-32/obj-i686-pc-linux-gnu/toolkit/xre/../../dist/include/mozilla/mozalloc.h:225 (discriminator 1))
16:14.29 XRE_main (/home/ehsan/mozilla-central-32/toolkit/xre/nsAppRunner.cpp:4301)
16:14.30 do_main (/home/ehsan/mozilla-central-32/browser/app/nsBrowserApp.cpp:282)
16:14.31 main (/home/ehsan/mozilla-central-32/browser/app/nsBrowserApp.cpp:643)
16:14.31 [39933] ###!!! ABORT: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file /home/ehsan/mozilla-central-32/xpcom/build/../glue/nsCOMPtr.h, line 870
16:14.31 Hit MOZ_CRASH() at /home/ehsan/mozilla-central-32/memory/mozalloc/mozalloc_abort.cpp:30

Is it expected for the cycle collector to finalize things this late during shutdown?

Comment 6

[Andrew McCreight [:mccr8]]

> Is it expected for the cycle collector to finalize things this late during shutdown?

Yes, that's just the place we trigger shutdown CCs.  I think that only happens precisely there in debug builds.

Comment 7

[(no longer active)]

Attachment: Null check the observer service in nsDocument::OnPageHide; r=mrbkap

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8417128 [details] [diff] [review]
Null check the observer service in nsDocument::OnPageHide; r=mrbkap

nsIPrincipal* principal = ...

Comment 9

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2b72314d24

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d2b72314d24

Comment 11

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Looking for instances of this crash over the past 4 weeks in Firefox 32, 33 and 34 [1], I could only find 2 crashes on Firefox 32. I think it's enough to call this verified.

[1] - https://crash-stats.mozilla.com/report/list?signature=nsDocument%3A%3AOnPageHide%28bool%2C+mozilla%3A%3Adom%3A%3AEventTarget%2A%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A34.0a1&version=Firefox%3A33.0a2&version=Firefox%3A32.0b&hang_type=any&date=2014-08-14+14%3A00%3A00&range_value=4#tab-reports