Closed Bug 1455964 Opened 2 years ago Closed 2 years ago

Crash in OOM | large | NS_ABORT_OOM | mozilla::EncodeLZ4<T>

Categories

(WebExtensions :: General, defect, critical)

58 Branch
All
Windows
defect
Not set
critical

Tracking

(firefox-esr52 unaffected, firefox-esr60 fixed, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: philipp, Assigned: kmag)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-78e111ff-18ae-44ce-9644-4a22b0180422.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll NS_ABORT_OOM xpcom/base/nsDebugImpl.cpp:614
1 xul.dll mozilla::EncodeLZ4<char [16]> toolkit/mozapps/extensions/AddonManagerStartup.cpp:155
2 xul.dll mozilla::AddonManagerStartup::EncodeBlob toolkit/mozapps/extensions/AddonManagerStartup.cpp:640
3 xul.dll NS_InvokeByIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54
4 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1234
5 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:913
6 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:468
7 xul.dll InternalCall js/src/vm/Interpreter.cpp:517
8 xul.dll Interpret js/src/vm/Interpreter.cpp:3086
9 xul.dll js::RunScript js/src/vm/Interpreter.cpp:418

=============================================================

this is a fairly low-volume crash that started showing up starting with firefox 58. the oom allocation size is usally around 400KB.
Attachment #8970029 - Flags: review?(aswan) → review?(tomica)
Comment on attachment 8970029 [details]
Bug 1455964: Make encodeBlob fallible on OOM.

https://reviewboard.mozilla.org/r/238782/#review244948

::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:163
(Diff revision 1)
> +  }
>  
>    size = LZ4::compress(data.BeginReading(), data.Length(),
>                         result.BeginWriting() + off);
>  
> -  result.SetLength(off + size);
> +  if (!result.SetLength(off + size, fallible)) {

This should always be shorter or equal to the last `setLength`, right?  Can that ever fail?
Attachment #8970029 - Flags: review?(tomica) → review+
https://hg.mozilla.org/mozilla-central/rev/5336e6fd895b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → kmaglione+bmo
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Seems like something we could fix in 60.1esr?  Please request uplift if you agree.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8970029 [details]
Bug 1455964: Make encodeBlob fallible on OOM.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: This causes an OOM crash (mostly on 32 bit operating systems) in cases that can be handled non-fatally.
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): None. This simply changes an OOM crash into a non-fatal JS exception.
String or UUID changes made by this patch: None.
Flags: needinfo?(kmaglione+bmo)
Attachment #8970029 - Flags: approval-mozilla-esr60?
Comment on attachment 8970029 [details]
Bug 1455964: Make encodeBlob fallible on OOM.

fix avoidable oom crash, approved for 60.1esr
Attachment #8970029 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.