833143 - Don't GC in nsXREDirProvider::DoShutdown()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

For some reason we force a GC (but not CC) in nsXREDirProvider::DoShutdown().
Just running one GC may not release too much.

Based on the blame the code has been there way before cycle collector was implemented
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/xre/nsXREDirProvider.cpp&rev=1.69&mark=827,849#827

Comment 1

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

More recent version of the code http://hg.mozilla.org/mozilla-central/annotate/425835f6a9e2/toolkit/xre/nsXREDirProvider.cpp#l838

Comment 2

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

Benjamin, by any chance, do you recall the reason for the GC?
Is that Phase 2c really needed and why?

Comment 3

[Benjamin Smedberg]

No, this was a port of the old seamonkey startup code in the Firefox 0.9.3 cycle, and so I kept this out of caution, not because I think we need it.

Comment 4

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

Attachment: don't GC

Crossing fingers try results will look ok
https://tbpl.mozilla.org/?tree=Try&rev=e278f5a296fb

(though, I really don't see any reasons for that GC.)

Comment 5

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

Originally the JS_GC is from Bug 97622.
The reasoning to add GC calls was weak https://bugzilla.mozilla.org/show_bug.cgi?id=97622#c66
The crashes should have been fixed in some other way.

Comment 6

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

Kaie, perhaps you have some comments to this?

Comment 7

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

I guess the problems related to NSS shutdown were fixed in Bug 189205, and the other problem
was about autocomplete+mork which isn't a combination used anymore.

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/689690a17de3

Comment 9

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

And backed out. WebRTC code has some bug...
https://hg.mozilla.org/mozilla-central/rev/680e46fecff0

Comment 10

[Benoit Girard (:BenWa)]

This is, unsurprising, the major contributor for a few of the slow shutdown profiles collected recently.

Comment 11

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

Yup. Can't land the patch before WebRTC (bug 834383) is fixed.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/31268d71c33c

Crossing fingers. Maybe better luck this time. Tryserver looked ok.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for bug 837843.

https://hg.mozilla.org/mozilla-central/rev/2360c3c46aca

Comment 15

[brunoais]

Is this available again to try to fix?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Better to ask smaug that.

Comment 17

[Andrew McCreight [:mccr8]]

FWIW, about:compartments doesn't exist any more. Of course, nobody figured out what the problem was, so there could be other lurking problems.

Comment 18

[brunoais]

How can we know if we don't try to merge the code again and see if someone finds a failure?

Comment 19

[Andrew McCreight [:mccr8]]

Well, somebody should at least try opening various about:pages and see if it hangs, with the patch applied.

Comment 20

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

brunoais, maybe you could test this? The patch applies to m-c with --fuzz=6

Comment 21

[brunoais]

Sorry. I don't know the terminology well.
What does "m-c" mean?

Is "--fuzz=6" the command line argument to add to "activate" this merge?

Comment 22

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

I mean could you build latest version of mozilla-central (m-c) with the patch.
To patch the source code you'd need to run for example
patch -p1 --fuzz=6 < /path/to/the/patch

Comment 23

[brunoais]

Where can I find the program called "patch"?

Comment 24

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

Ok, perhaps it is easier if I just upload the patch to try and you can try the binaries :)

Wait for an hour or two, and check https://tbpl.mozilla.org/?tree=Try&rev=947d95a6a657 whether
the builds are ready (no need to wait all the tests to be ready), and after that they should show up 
in http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/?C=N;O=A
Look for 947d95a6a657 there.

Comment 25

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

Builds are here http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-947d95a6a657/

Comment 26

[brunoais]

I have been experimenting with a copy of my main firefox profile and it seems to be working as expected. I'll try now to keep it on for a full day and use it a bit in the mean time.

Comment 27

[brunoais]

I've been trying through all this days as a secondary browser. I've been using almost as a mirror or my main version (current stable one) (side-by-side and repeating operations except in things that should only be done once).

It ran as expected and without problems. So I don't know if I was really useful or not because I was unable to break it... I hope I was.

Comment 28

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

Sounds like we could try landing the patch again.
(In theory the patch itself is just fine, and if it causes problems, then issues are elsewhere. )

Keeping the needinfo so that I don't forget to land the patch.

Comment 29

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/006353e45fa7

Comment 30

[Ed Morley [:emorley]]

Reverted for robocop crashes:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=006353e45fa7&jobname=robocop

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d014b12e6afc

Comment 31

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

Really odd looking crash. Dalvik doesn't like the change?

Comment 32

[Andrew McCreight [:mccr8]]

The crashes are all inside Java_org_mozilla_gecko_GeckoAppShell_notifyBatteryChange.  I wonder if there's some kind of battery notification thing that is normally getting shut down by this last GC, but with your patch it isn't, so very deep in shutdown we get a battery notification and but the event loop is gone or whatever and we crash.

Comment 33

[Andrew McCreight [:mccr8]]

kats, do you know anything about this notifyBatteryCharge thing and how it is supposed to be shut down, or who might know about it?  Thanks.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

I don't know too much about it but from looking at the code that notification should not get fired if the notifications are disabled by calling [1]. Working backwards that should happen once all the observers are unregistered from the sBatteryObservers manager in Hal.cpp, by calling [2]. There's a handful of callers to this [3] so perhaps it would be easiest to add logging to those callers and see which one fires when we have a GC but doesn't fire without the GC. Or if you're familiar with those call sites you could do a code inspection and maybe figure it out.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoBatteryManager.java?rev=2b5e76962106#190
[2] http://mxr.mozilla.org/mozilla-central/source/hal/Hal.cpp?rev=326396f9cc4e#350
[3] http://mxr.mozilla.org/mozilla-central/ident?i=UnregisterBatteryObserver

Comment 35

[Andrew McCreight [:mccr8]]

It sounds like this is causing a common source of shutdown hangs, so it might be worth doing a full try push and see how things are.

Comment 36

[Andrew McCreight [:mccr8]]

I guess I'll do that this week. If Android is still a problem, we can just leave this GC on Android. There's probably less to clean up there, so it shouldn't be as much of an issue.

Comment 37

[Andrew McCreight [:mccr8]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83b53f8d4bec1fe94e11ccb8b9574251f0c255cc

Comment 38

[Andrew McCreight [:mccr8]]

Android looked good. I only saw one failure, bug 833143, which was just a minor test problem, which I have a patch for.

Comment 39

[Andrew McCreight [:mccr8]]

Attachment: Don't GC in nsXREDirProvider::DoShutdown

Comment 40

[Andrew McCreight [:mccr8]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=686633edd3a68ac6eeb8e10dc0e4e3532a0bbce1

Comment 41

[Andrew McCreight [:mccr8]]

Note to sheriffs: this may cause new intermittent leaks.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e01d4124eae
Don't GC in nsXREDirProvider::DoShutdown. r=bsmedberg, r=billm

Comment 43

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1355480 and bug 833143 on suspicion of triggering assertion CompositorThreadHolder::IsInCompositorThread() on Windows 8 x64 M-e10s:

Bug 1355480
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e002af2f7019f8b45a05722efb882043962ae75

Bug 833143
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb15e1b1baee587c057ecc907759e223bf421315

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5e01d4124eaefeb0702f31689a9223f14351368e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=96034710&repo=mozilla-inbound

13:02:10     INFO - GECKO(1800) | Assertion failure: CompositorThreadHolder::IsInCompositorThread(), at c:/builds/moz2_slave/m-in-w64-d-0000000000000000000/build/src/gfx/layers/AnimationHelper.cpp:28
13:02:10     INFO - GECKO(1800) | #01: mozilla::layers::CompositorAnimationStorage::Release() [obj-firefox/dist/include/mozilla/layers/AnimationHelper.h:83]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #02: mozilla::layers::CompositorBridgeParent::ActorDestroy(mozilla::ipc::IProtocol::ActorDestroyReason) [gfx/layers/ipc/CompositorBridgeParent.cpp:644]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #03: mozilla::layers::PCompositorBridgeParent::OnChannelClose() [obj-firefox/ipc/ipdl/PCompositorBridgeParent.cpp:1808]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #04: mozilla::ipc::MessageChannel::NotifyChannelClosed() [ipc/glue/MessageChannel.cpp:2651]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #05: mozilla::ipc::MessageChannel::OnNotifyMaybeChannelError() [ipc/glue/MessageChannel.cpp:2512]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #06: mozilla::detail::RunnableMethodImpl<mozilla::ipc::MessageChannel * const,void ( mozilla::ipc::MessageChannel::*)(void),0,1>::Run() [xpcom/threads/nsThreadUtils.h:911]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #07: MessageLoop::RunTask(already_AddRefed<mozilla::Runnable>) [ipc/chromium/src/base/message_loop.cc:362]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #08: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask &&) [ipc/chromium/src/base/message_loop.cc:372]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #09: MessageLoop::DoWork() [ipc/chromium/src/base/message_loop.cc:444]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #10: base::MessagePumpForUI::DoRunLoop() [ipc/chromium/src/base/message_pump_win.cc:213]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #11: base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate *,base::MessagePumpWin::Dispatcher *) [ipc/chromium/src/base/message_pump_win.cc:58]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #12: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:232]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #13: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:212]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #14: base::Thread::ThreadMain() [ipc/chromium/src/base/thread.cc:182]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #15: `anonymous namespace'::ThreadFunc [ipc/chromium/src/base/platform_thread_win.cc:29]
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #16: KERNEL32.DLL + 0x167e
13:02:10     INFO - 
13:02:10     INFO - GECKO(1800) | #17: ntdll.dll + 0x1c3f1

Comment 44

[Andrew McCreight [:mccr8]]

There's a similar looking assertion failure on OSX in the failure link in comment 43. I'm unable to reproduce locally. I'll see if it shows up in a retrigger. There's no stack, which might make it harder to figure out. Bug 1361825 (and the possible OS X issue) feels like they are just an existing race between IPDL ActorDestroy (for Compositor or GMP) and thread shutdown.

Comment 45

[Andrew McCreight [:mccr8]]

This looks green on OSX and Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7474ea38e9ed8a5a946cc17a42a06fdc75d42472

Maybe some existing code fixed the compositor shutdown race or whatever it was.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2efdd8dcd82
Don't GC in nsXREDirProvider::DoShutdown. r=bsmedberg, r=billm

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 48

[Andrew McCreight [:mccr8]]

Comment on attachment 8863040 [details] [diff] [review]
Don't GC in nsXREDirProvider::DoShutdown

Approval Request Comment
[Feature/Bug causing the regression]: very old
[User impact if declined]: This fixes a major source of shutdown hangs. I see almost 10,000 crashes across all versions that have shutdownhang in the signature and JS_GC in the proto signature, and this patch should fix at least some of them. I see 160k shutdownhang crashes that do NOT contain JS_GC, so maybe that's about 6.25% of all shutdown hangs.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: It has been in Nightly for a couple of weeks. No regressions have been reported, and no shutdown hangs with JS_GC in the proto signature have been reported since then.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Somewhat.
[Why is the change risky/not risky?]:  Changing shutdown is always a little risky, and this patch did get backed out a few times over the years for weird issues in debug builds (that were detected in automation).
[String changes made/needed]: none

Comment 49

[Andrew McCreight [:mccr8]]

> so maybe that's about 6.25% of all shutdown hangs.
Sorry, my math was wrong. That should be "about 5.8% of all shutdown hangs".

Comment 50

[Julien Cristau [:jcristau]]

Comment on attachment 8863040 [details] [diff] [review]
Don't GC in nsXREDirProvider::DoShutdown

I think given the age of this bug it doesn't need to skip trains.