1257216 - Increase toolkit.asyncshutdown.crash_timeout for winXP

Status: RESOLVED WONTFIX

(Toolkit :: General, defect)

Description

[Dragana Damjanovic [:dragana]]

We have some crashes on shutdown crashes on winxp in necko - bug 1158189 (comment 75).

I would like to try to increase toolkit.asyncshutdown.crash_timeout and see if this will help. 
It will be done only for XP and we can also revert it after 1-2 week.

Comment 1

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12629f1cc4b1

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: bug_1257216_v1.patch

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8731306 [details] [diff] [review]
bug_1257216_v1.patch

Review of attachment 8731306 [details] [diff] [review]:
-----------------------------------------------------------------

Fine with me, if this is only for XP, but this doesn't increase toolkit.asyncshutdown.timeout.crash, just the Terminator shutdown duration.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this may help with a topcrash in 46.0b2.

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_1257216_v1.patch

Sorry for the second patch. This patch should change timeout only on win XP.

Comment 6

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=438094551c39

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Apologies for the delay, this was a crazy (work)week. I'm on it now.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8733971 [details] [diff] [review]
bug_1257216_v1.patch

Review of attachment 8733971 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not really happy about these code changes, because they introduce weirdness in code that is performance-critical.

If the code is meant to live only for a few weeks, I'm willing to go ahead with it, but in this case, please mention it in the code & extended commit message. r=me with that, if the code is backed out soon.

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ +77,5 @@
>  
>  
>  // Crash the process if shutdown is really too long
>  // (allowing for sleep).
> +var PREF_DELAY_CRASH_MS = "toolkit.asyncshutdown.crash_timeout";

AsyncShutdown is safety-critical, so I'd rather avoid special-casing stuff, as this will make it harder to test and trust.

Is there any way you could perform the change somewhere else?

::: toolkit/components/terminator/nsTerminator.cpp
@@ +376,5 @@
>  void
>  nsTerminator::StartWatchdog()
>  {
> +  int32_t crashAfterMS;
> +#if defined(XP_WIN32)

Likewise, the nsTerminator is safety-critical, so I'd really prefer if it was not special-cased.

Comment 9

[Dragana Damjanovic [:dragana]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #8)
> Comment on attachment 8733971 [details] [diff] [review]
> bug_1257216_v1.patch
> 
> Review of attachment 8733971 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not really happy about these code changes, because they introduce
> weirdness in code that is performance-critical.
> 
> If the code is meant to live only for a few weeks, I'm willing to go ahead
> with it, but in this case, please mention it in the code & extended commit
> message. r=me with that, if the code is backed out soon.
> 
> ::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
> @@ +77,5 @@
> >  
> >  
> >  // Crash the process if shutdown is really too long
> >  // (allowing for sleep).
> > +var PREF_DELAY_CRASH_MS = "toolkit.asyncshutdown.crash_timeout";
> 
> AsyncShutdown is safety-critical, so I'd rather avoid special-casing stuff,
> as this will make it harder to test and trust.
> 
> Is there any way you could perform the change somewhere else?
> 
> ::: toolkit/components/terminator/nsTerminator.cpp
> @@ +376,5 @@
> >  void
> >  nsTerminator::StartWatchdog()
> >  {
> > +  int32_t crashAfterMS;
> > +#if defined(XP_WIN32)
> 
> Likewise, the nsTerminator is safety-critical, so I'd really prefer if it
> was not special-cased.


I was looking at the code, but I  do not know how that really works. So I will ask bsmedberg(I have seen you review a lot of patches in that code):

I would like "toolkit.asyncshutdown.crash_timeout"  pref to have a different default value only on winXP and Vista (3min on xp and vista and 1min on all other). Is it possible to set that up at the start time and have only one visible pref? Sorry, I am not sure how this all work.

Comment 10

[Benjamin Smedberg]

I don't understand why this is valuable at all. Even if there are fewer shutdown timeouts with a 3-minute timeout, that's not something we'd do in release anyway. It's better to crash than to shutdown-hang for longer.

Comment 11

[Dragana Damjanovic [:dragana]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> I don't understand why this is valuable at all. Even if there are fewer
> shutdown timeouts with a 3-minute timeout, that's not something we'd do in
> release anyway. It's better to crash than to shutdown-hang for longer.

In most of the cases we hang in PR_Connect which actually hangs in Windows os function in the most of the cases WSPBind, but not always. It is not really understandable why it is hanging there. Some times viruses or antivirus are involved, sometimes not. Most of the time winXP

So we do not have much options, for now. Making a shutdown long is to see whether the system is just slow, or there is a problem in windows os. (We are also considering other options, and investigating further...)

I wanted this only for test. I could probably use this patch for test, because it is only a test for a short time.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Dragana, we are heading into beta 10 now. Are you still planning to land this test patch?

Comment 13

[Dragana Damjanovic [:dragana]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Hi Dragana, we are heading into beta 10 now. Are you still planning to land
> this test patch?

Would you accept this patch for a test just for one build and back it out in beta 11?
Should it be landed on m-c first or not?

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, we could land it in beta 10 and back it out for beta 11. Can you request uplift ? I don't think we have to land this on all branches since it's specifically testing the issue on beta.
Also, I think opening a new bug for the backout will make things more clear

Comment 15

[Dragana Damjanovic [:dragana]]

Comment on attachment 8733971 [details] [diff] [review]
bug_1257216_v1.patch

Approval Request Comment
[Feature/regressing bug #]: Testing a issue with shutdown hang.
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8733971 [details] [diff] [review]
bug_1257216_v1.patch

Diagnostic patch for XP shutdown hang/crash, please land for beta 10. 
We will back this out in beta 11.

Comment 17

[Carsten Book [:Tomcat]]

has problems to apply:

patching file modules/libpref/init/all.js
Hunk #1 FAILED at 848
1 out of 1 hunks FAILED -- saving rejects to file modules/libpref/init/all.js.rej
patching file toolkit/crashreporter/test/unit/test_crash_AsyncShutdown.js
Hunk #2 succeeded at 53 with fuzz 1 (offset 0 lines).
Hunk #3 succeeded at 99 with fuzz 1 (offset 0 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug_1257216_v1.patch

can you take a look, thanks!

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: bug_1257216_v1.patch

rebased patch

Comment 19

[Dragana Damjanovic [:dragana]]

(In reply to Dragana Damjanovic [:dragana] from comment #18)
> Created attachment 8739919 [details] [diff] [review]
> bug_1257216_v1.patch
> 
> rebased patch

I cannot set  beta approval, but it should be approved because it is only rebased version.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9ea83990839b

setting leave open since i  guess we can to backout this at some time

Comment 21

[Dragana Damjanovic [:dragana]]

(In reply to Carsten Book [:Tomcat] from comment #20)
> https://hg.mozilla.org/releases/mozilla-beta/rev/9ea83990839b
> 
> setting leave open since i  guess we can to backout this at some time

I have open a separate bug for backing this out: bug 1263328.

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Dragana Damjanovic [:dragana] from comment #21)
> (In reply to Carsten Book [:Tomcat] from comment #20)
> > https://hg.mozilla.org/releases/mozilla-beta/rev/9ea83990839b
> > 
> > setting leave open since i  guess we can to backout this at some time
> 
> I have open a separate bug for backing this out: bug 1263328.

ok cool thanks :) then setting this as fixed :)

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Wes or Tomcat, can you back out the beta patch now so we don't ship this in beta 11? Thanks!

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/18b4c47bc1c0

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Setting 46 status to wontfix since we aren't shipping with the test patch.