Crash in AsyncShutdownTimeout | profile-before-change | UpdateManager: writing update xml data

RESOLVED FIXED in Firefox 65

Status

()

defect
P1
critical
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: baffclan, Assigned: rstrong)

Tracking

({crash})

Trunk
mozilla66
x86_64
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox64 wontfix, firefox65 fixed, firefox66 fixed)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

This bug was filed from the Socorro interface and is
report bp-8822c35a-a568-460d-bd48-50f240181015.
=============================================================

Top 10 frames of crashing thread:

0 mozglue.dll mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:35
1 xul.dll static void Abort xpcom/base/nsDebugImpl.cpp:471
2 xul.dll NS_DebugBreak xpcom/base/nsDebugImpl.cpp:453
3 xul.dll nsDebugImpl::Abort xpcom/base/nsDebugImpl.cpp:146
4 xul.dll XPTC__InvokebyIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm:97
5  @0x1c302210abf 
6 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1233
7 xul.dll static bool XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1019
8 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:553
9 xul.dll static bool InternalCall js/src/vm/Interpreter.cpp:607

=============================================================
Component: Memory Allocator → Application Update
Product: Core → Toolkit
Duplicate of this bug: 1513158
Note: there are a couple of things we can do.
1. change the delay to 0 for writing out the xml files when the quit-application notification is received.
2. make it so saveUpdates isn't called during a download onStopRequest unless it really is necessary which it isn't in most cases.
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Priority: -- → P1
Attachment #9034942 - Flags: review?(mhowell) → review+
Comment on attachment 9034942 [details] [diff] [review]
patch rev1

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2922,5 @@
>          wroteSuccessfully => handleCriticalWriteResult(wroteSuccessfully,
>                                                         FILE_UPDATES_XML)
>        );
>      }
> +    return Promise.all(promises);

I see that _saveUpdatesXML is being called from the task passed to DeferredTask, but I don't see anything there using this return value.
Comment on attachment 9034942 [details] [diff] [review]
patch rev1

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ -2892,5 @@
>     * Saves the active-updates.xml and updates.xml when the updates history has
>     * been modified files.
>     */
>    _saveUpdatesXML: function UM__saveUpdatesXML() {
> -    AsyncShutdown.profileBeforeChange.removeBlocker(this._updatesXMLSaverCallback);

This line has been removed but the addBlocker line still remains. Is this intentional?

@@ +2922,5 @@
>          wroteSuccessfully => handleCriticalWriteResult(wroteSuccessfully,
>                                                         FILE_UPDATES_XML)
>        );
>      }
> +    return Promise.all(promises);

I now see that DeferredTask uses this promise. Sorry for the confusion. I chatted with felipe and both of us agree that this code is fine.
Comment on attachment 9034942 [details] [diff] [review]
patch rev1

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ -2892,5 @@
>     * Saves the active-updates.xml and updates.xml when the updates history has
>     * been modified files.
>     */
>    _saveUpdatesXML: function UM__saveUpdatesXML() {
> -    AsyncShutdown.profileBeforeChange.removeBlocker(this._updatesXMLSaverCallback);

Nevermind, this is fine. The `if (!this._updatesXMLSaver)` condition prevents this from getting added too many times.

@@ +2922,5 @@
>          wroteSuccessfully => handleCriticalWriteResult(wroteSuccessfully,
>                                                         FILE_UPDATES_XML)
>        );
>      }
> +    return Promise.all(promises);

I now see that DeferredTask uses this promise. Sorry for the confusion. I chatted with felipe and both of us agree that this code is fine.
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9721f08cfc14
fix for the crash in AsyncShutdownTimeout | profile-before-change | UpdateManager: writing update xml data. r=mhowell
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Given the frequency of the crash, size of the patch, and where we are in the cycle, I think this should ride the trains. Feel free to rebase the patch (it doesn't graft cleanly) and nominate for Beta approval if you feel strongly otherwise, however.

Besides the crash, this bug makes it so telemetry for write failures isn't reported under some cases so I'd like to get this in beta.

Attachment #9035381 - Flags: review?(mhowell)

Comment on attachment 9035381 [details] [diff] [review]
client patch for beta

Note: I minimized the client changes.

The test patch just fixes merge conflicts

Attachment #9035382 - Flags: review?(mhowell)
Attachment #9035381 - Flags: review?(mhowell) → review+
Attachment #9035382 - Flags: review?(mhowell) → review+

Comment on attachment 9035381 [details] [diff] [review]
client patch for beta

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1361102

User impact if declined: The crash will continue and we won't get telemetry for write failures.
Note: bug 1458314 is also involved in the regression

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: Clients updating should be enough.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The patch was checked by 3 devs and numerous tests hit this code path.

String changes made/needed: N/A

Attachment #9035381 - Flags: approval-mozilla-beta?

Comment on attachment 9035381 [details] [diff] [review]
client patch for beta

[Triage Comment]
Fixes a crash and, under some circumstances, broken Telemetry collection for write failures. Has good automated test coverage (thanks for that). Approved for 65.0b10.

Attachment #9035381 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.