Temporarily enable assertion in isalloc_validate in release builds, to test for potential ozone_size corruption

VERIFIED FIXED in Firefox 11

Status

()

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

({verified-beta})

unspecified
mozilla11
x86
macOS
verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11+ verified)

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 1 obsolete attachment)

See bug 702250 comment 41: We're concerned that there might be jemalloc problems on 10.6 and 10.7 that somehow don't manifest themselves as a crash.

We can test this with minimal overhead by running an assertion in isalloc_validate on release builds.  If we see crashes at the assertion, then we can take appropriate action, such as panicking.
(Assignee)

Updated

7 years ago
Blocks: 414946
(Assignee)

Updated

7 years ago
Blocks: 694896
(Assignee)

Updated

7 years ago
Assignee: nobody → justin.lebar+bug
Try run for a025661fb603 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a025661fb603
Results (out of 35 total builds):
    success: 26
    warnings: 8
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-a025661fb603
(Assignee)

Updated

7 years ago
Attachment #575004 - Attachment is obsolete: true
Try run for 70887ea960e2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=70887ea960e2
Results (out of 57 total builds):
    success: 32
    warnings: 24
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-70887ea960e2
(Assignee)

Updated

7 years ago
Attachment #575304 - Attachment description: WIP v2 → Patch v2
Attachment #575304 - Flags: review?(khuey)
Landed and then backed out, because Windows was unhappy with

  if (...) {
    _malloc_message(...);
    char* boom = 0;
  }

But rather than give me a meaningful error about mixing declarations and code, it just barfed at me.

Anyway, it builds if I reorder the two lines, so I'll push that, unless there are objections.
https://hg.mozilla.org/mozilla-central/rev/7653280aa252
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Nominating for tracking FF11 because:

 * We almost surely want to back out before this hits beta, since it may cause crashes.

 * We need to monitor crash-stats carefully, because crashes here indicate at the very least that we'd return the wrong value from ozone_size, and may indicate memory corruption.
status-firefox11: --- → fixed
tracking-firefox11: --- → ?
It's been almost three weeks since this bug's patch landed.  In that time there haven't been any crashes with isalloc_validate as the signature on the FF 11 branch, on any platform.  So things are looking good.

Presuming of course that Breakpad is behaving as expected :-)
(In reply to Steven Michaud from comment #8)
> Presuming of course that Breakpad is behaving as expected :-)

That might not be the case: see bug 708453.
So it looks like Breakpad reports stopped being sent "automatically" on Mac trunk as of the 2011-11-23 mozilla-central nightly.  (Any crash reports for trunk builds made since then were sent as the result of explicit user intervention -- for example viewing about:crashes and clicking on one of the links.)

It seems bug 708453 will be fixed soon.  I'll check again in another 2-3 weeks :-)
> I'll check again in another 2-3 weeks :-)

Let's check a few days before Dec. 20, at the latest?

Thanks for being on top of this.
> Let's check a few days before Dec. 20, at the latest?

Sounds fine to me.  I'm not on vacation until Dec 23rd.
How's this look now?
No crashes at isalloc_validate on the FF 11 branch in the last week.

But let's give it another week.  There aren't many users on the trunk.
No crashes at isalloc_validate on the FF 11 branch in the last two weeks.

So this patch can probably be backed out.

But you could argue that we *should* let it into at least one beta, because betas have many more users, and some bugs require lots of users to unearth.  A recent case in point is bug 711794.
I guess letting it bake in beta is fair, since the crash we previously saw in here was pretty hard to reproduce.  But on the other hand, we have at this point plenty of reason to believe that this assertion won't be tripped on 10.6+.
Tracking for FF11 to back out after 11.0beta1 at the latest.
tracking-firefox11: ? → +
qa+ to track for QA during Firefox 11 Beta cycle -- however no explicit "fix verification" is needed at this point.
Whiteboard: [qa+]
Verified again the crash reports for Firefox 11 beta and there was no crash with the isalloc_validate signature.
Marking this VERIFIED for now, will reopen if it's the case.
Status: RESOLVED → VERIFIED
status-firefox11: fixed → verified
Keywords: verified-beta
Whiteboard: [qa+] → [qa!]
(Assignee)

Updated

7 years ago
Blocks: 731696
We're backing this out as part of bug 731696 (just landed on m-i).  I guess this bug can stay FIXED, since it's nominally about "temporarily" enabling an assertion.
You need to log in before you can comment on or make changes to this bug.