1269185 - startup crash in nsZipHandle::findDataStart

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[[:philipp]]

[Tracking Requested - why for this release]:
nominating for tracking since it's a recent regression

This bug was filed from the Socorro interface and is 
report bp-43fd92e8-1556-4540-ac47-891212160428.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsZipHandle::findDataStart() 	modules/libjar/nsZipArchive.cpp
1 	xul.dll 	nsZipHandle::Init(nsIFile*, nsZipHandle**, PRFileDesc**) 	modules/libjar/nsZipArchive.cpp
2 	xul.dll 	nsZipArchive::OpenArchive(nsIFile*) 	modules/libjar/nsZipArchive.cpp
3 	xul.dll 	mozilla::scache::StartupCache::LoadArchive(mozilla::scache::StartupCache::TelemetrifyAge) 	startupcache/StartupCache.cpp
4 	xul.dll 	mozilla::scache::StartupCache::Init() 	startupcache/StartupCache.cpp
5 	xul.dll 	mozilla::scache::StartupCache::InitSingleton() 	startupcache/StartupCache.cpp
6 	xul.dll 	mozilla::scache::StartupCache::GetSingleton() 	startupcache/StartupCache.cpp
7 	xul.dll 	NS_InitXPCOM2 	xpcom/build/XPCOMInit.cpp
8 	xul.dll 	ScopedXPCOMStartup::Initialize() 	toolkit/xre/nsAppRunner.cpp
9 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
10 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
11 	xul.dll 	nsAString_internal::ReplacePrep(unsigned int, unsigned int, unsigned int) 	xpcom/string/nsTSubstring.cpp
12 	firefox.exe 	firefox.exe@0x151d3

this new crash signature starts to show up in 47.0b1 and it's occurring on startup. it looks related to the work done in bug 1260836.

Comment 1

[Benjamin Smedberg]

In some parts of libjar, we use Windows SEH to trap reads from mapped memory and turn those into errors: see e.g. https://hg.mozilla.org/releases/mozilla-beta/annotate/5bbf2e7c2fc6/modules/libjar/nsZipArchive.cpp#l447

I don't know whether, during this refactoring, you just moved an existing crash to another function (which is probably not a big deal). But if you moved access outside of an SEH block, it may be simple to re-add that and propagate errors instead of crashing. Antivirus is a common cause for memory-mapped files to "disappear" causing these IN_PAGE_ERROR exceptions.

Comment 2

[Robin Steuber (they/them) [:bytesized]]

That explains a lot. I was just in the middle of writing a comment noting how these crashes have been seen before (in a different function), but mostly in OSX whereas now we are seeing them in Windows.

I will write a patch to add the windows exception handling. Currently, all crashes with the signature nsZipHandle::findDataStart are in Windows, so this should fix that problem. However, I feel that it is worth pointing out that this bug still gets hit in other operating systems. Before I added nsZipHandle::findDataStart, the first place where the zip file is read is in nsZipArchive::BuildFileList [1]. Looking for crashes with this signature yields a number of crashes in OSX [2]. These have the same stack trace, just crashing in nsZipArchive::BuildFileList instead of nsZipHandle::findDataStart.

[1] https://hg.mozilla.org/releases/mozilla-release/annotate/e35da3da61cb/modules/libjar/nsZipArchive.cpp#l625
[2] https://crash-stats.mozilla.com/report/index/6ebcea83-ce6b-481f-b14b-181572160428

Comment 3

[Robin Steuber (they/them) [:bytesized]]

Attachment: MozReview Request: Bug 1269185 - Prevent crashes in Windows when zip files cannot be read

Review commit: https://reviewboard.mozilla.org/r/50551/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50551/

Comment 4

[Stephen A Pohl [:spohl] (OOO until 6/12)]

Comment on attachment 8748827 [details]
MozReview Request: Bug 1269185 - Prevent crashes in Windows when zip files cannot be read

https://reviewboard.mozilla.org/r/50551/#review47517

Comment 5

[Robin Steuber (they/them) [:bytesized]]

Any interest in uplifting this, given that Bug 1260836 (Add support for CRX) was uplifted?

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3acf01a84cb

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Kirk Steuber [:ksteuber] from comment #5)
> Any interest in uplifting this, given that Bug 1260836 (Add support for CRX)
> was uplifted?

Yes! Thanks for the ni?

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8748827 [details]
MozReview Request: Bug 1269185 - Prevent crashes in Windows when zip files cannot be read

Approval Request Comment
[Feature/regressing bug #]: Widevine EME
[User impact if declined]: Crashes when we fail to install the Widevine CDM
[Describe test coverage new/current, TreeHerder]: We have tests covering the Widevine CDM installer
[Risks and why]: Low; just adding error checking
[String/UUID change made/needed]: None.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8748827 [details]
MozReview Request: Bug 1269185 - Prevent crashes in Windows when zip files cannot be read

Fix a crash during Widevine CDM installation, Aurora48+, Beta47+

Comment 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/36f6b5aa4e05

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a0c1f2f189a2