All users were logged out of Bugzilla on October 13th, 2018

Increase toolkit.asyncshutdown.crash_timeout for winXP

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

unspecified
x86
Windows XP
Points:
---

Firefox Tracking Flags

(firefox46+ wontfix)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 2

3 years ago
Created attachment 8731306 [details] [diff] [review]
bug_1257216_v1.patch
Attachment #8731306 - Flags: review?(dteller)
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.
Attachment #8731306 - Flags: review?(dteller) → review+
Tracking since this may help with a topcrash in 46.0b2.
status-firefox46: --- → affected
tracking-firefox46: --- → +
(Assignee)

Comment 5

3 years ago
Created attachment 8733971 [details] [diff] [review]
bug_1257216_v1.patch

Sorry for the second patch. This patch should change timeout only on win XP.
Attachment #8731306 - Attachment is obsolete: true
Attachment #8733971 - Flags: review?(dteller)
Apologies for the delay, this was a crazy (work)week. I'm on it now.
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.
Attachment #8733971 - Flags: review?(dteller) → review+
(Assignee)

Comment 9

3 years ago
(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.
Flags: needinfo?(benjamin)

Comment 10

3 years ago
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.
Flags: needinfo?(benjamin) → needinfo?(dd.mozilla)
(Assignee)

Comment 11

3 years ago
(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.
Flags: needinfo?(dd.mozilla)
Hi Dragana, we are heading into beta 10 now. Are you still planning to land this test patch?
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 13

3 years ago
(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?
Flags: needinfo?(dd.mozilla)
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
(Assignee)

Comment 15

3 years ago
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]:
Attachment #8733971 - Flags: approval-mozilla-beta?
(Assignee)

Updated

3 years ago
Blocks: 1263328
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.
Attachment #8733971 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 18

3 years ago
Created attachment 8739919 [details] [diff] [review]
bug_1257216_v1.patch

rebased patch
Flags: needinfo?(dd.mozilla)
Attachment #8739919 - Flags: review+
(Assignee)

Comment 19

3 years ago
(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.
https://hg.mozilla.org/releases/mozilla-beta/rev/9ea83990839b

setting leave open since i  guess we can to backout this at some time
status-firefox46: affected → fixed
Keywords: leave-open
(Assignee)

Comment 21

3 years ago
(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.
(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 :)
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Wes or Tomcat, can you back out the beta patch now so we don't ship this in beta 11? Thanks!
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)

Comment 24

3 years ago
backout
https://hg.mozilla.org/releases/mozilla-beta/rev/18b4c47bc1c0
status-firefox46: fixed → affected
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
Setting 46 status to wontfix since we aren't shipping with the test patch.
status-firefox46: affected → wontfix
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.