Linux and OSX startup crash in Pickle::ReadUInt32

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Telemetry
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: froydnj)

Tracking

({crash, regression})

13 Branch
mozilla13
crash, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [startupcrash], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Only ranked 40, but given that it is mostly Linux that seems fairly high. 91% Linux, 9% OSX. 100% of crashes are after < 1 min. I'm guessing it could be a regression from bug 707320.

In the ones I was looking at, the pickle function is being called from LoadHistogramEvent::Run.

Here are some examples:
https://crash-stats.mozilla.com/report/index/240f7645-18c9-48d8-8499-4ad4b2120306
https://crash-stats.mozilla.com/report/index/5d1b91b2-0f62-45ad-9570-fcaa52120306

Comment 1

6 years ago
(In reply to Andrew McCreight [:mccr8] from comment #0)
> I'm guessing it could be a regression from bug 707320.
Not sure because there are crashes in 12.0a2/20120207 and 20120209.
Severity: normal → critical
Keywords: crash, regression
Whiteboard: [startupcrash]
Version: Trunk → 13 Branch
(In reply to Scoobidiver from comment #1)
> (In reply to Andrew McCreight [:mccr8] from comment #0)
> > I'm guessing it could be a regression from bug 707320.
> Not sure because there are crashes in 12.0a2/20120207 and 20120209.

We don't care about any crash earlier than 2012029, because it's not the same code.
OK, so I can reproduce this on my machine by doing something like:

echo 'fghk' > $PROFILE_DIRECTORY/sessionHistograms.dat.8little
firefox -P $PROFILE_NAME -new-instance

It looks like we need to check that the amount of file we read in roughly agrees with how much data Pickle thinks it has to read.  I'm not sure I can do that without getting my hands dirty with internals of Pickle.

This looks like it would take care of bug 722625 as well.
Created attachment 604108 [details] [diff] [review]
patch with more sanity-checking

Here's a patch for checking:

1) That the file is actually big enough to contain the minimum amount of data we need.  This wasn't actually causing the crash, but it is a latent bug.  (If we had 0-byte data files, then we would crash a little bit earlier than where this crash report is about.)

2) That the amount of data stored in the pickle is synonymous with how much data we know we read.  This check is an early check for too-short data files, but it also catches the cases where:

- We only wrote the pickle header, but didn't write anything else.  It therefore looks like there's a lot of data to read, but there's actually not.

- The pickle header is badly corrupt.

(I'm not totally happy with this check, as it can assert in debug builds with suitably corrupt datafiles, but I suppose that is the point of debug builds, yes?)
Attachment #604108 - Flags: review?(taras.mozilla)

Comment 5

6 years ago
Comment on attachment 604108 [details] [diff] [review]
patch with more sanity-checking

depressing that pickle doesn't do this. More and more reason to rewrite this code :(
Attachment #604108 - Flags: review?(taras.mozilla) → review+

Comment 6

6 years ago
Comment on attachment 604108 [details] [diff] [review]
patch with more sanity-checking

-  if (size == -1) {
+  if ((size_t)size < sizeof(Pickle::Header)) {

^-- seems bad, same sort of bad int->uint casting below.
Attachment #604108 - Flags: review+ → review-
Created attachment 604513 [details] [diff] [review]
patch with more sanity-checking

Now with C++-style casts and a little more thought put into how to deal with signed-unsigned comparisons.
Attachment #604108 - Attachment is obsolete: true
Attachment #604513 - Flags: review?(taras.mozilla)

Updated

6 years ago
Attachment #604513 - Flags: review?(taras.mozilla) → review+
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Whiteboard: [startupcrash] → [startupcrash][autoland-try:-b do -p all -u all -t none]

Updated

6 years ago
Whiteboard: [startupcrash][autoland-try:-b do -p all -u all -t none] → [startupcrash][autoland-in-queue]

Comment 8

6 years ago
Autoland Patchset:
	Patches: 604513
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=470be8b24966
Try run started, revision 470be8b24966. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=470be8b24966

Comment 9

6 years ago
Try run for 470be8b24966 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=470be8b24966
Results (out of 216 total builds):
    success: 179
    warnings: 36
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-470be8b24966

Updated

6 years ago
Whiteboard: [startupcrash][autoland-in-queue] → [startupcrash]
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9949b7477858
Keywords: checkin-needed
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/9949b7477858
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 722625
You need to log in before you can comment on or make changes to this bug.