Last Comment Bug 703087 - Temporarily enable assertion in isalloc_validate in release builds, to test for potential ozone_size corruption
: Temporarily enable assertion in isalloc_validate in release builds, to test f...
Status: VERIFIED FIXED
[qa!]
: verified-beta
Product: Core
Classification: Components
Component: Memory Allocator (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla11
Assigned To: Justin Lebar (not reading bugmail)
:
: Mike Hommey [:glandium]
Mentors:
Depends on:
Blocks: 414946 694896 731696
  Show dependency treegraph
 
Reported: 2011-11-16 14:07 PST by Justin Lebar (not reading bugmail)
Modified: 2012-03-05 11:09 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
WIP v1 (1.12 KB, patch)
2011-11-16 14:49 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (2.37 KB, patch)
2011-11-17 15:02 PST, Justin Lebar (not reading bugmail)
khuey: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-11-16 14:07:05 PST
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.
Comment 1 Justin Lebar (not reading bugmail) 2011-11-16 14:49:12 PST
Created attachment 575004 [details] [diff] [review]
WIP v1
Comment 2 Mozilla RelEng Bot 2011-11-16 17:10:22 PST
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
Comment 3 Justin Lebar (not reading bugmail) 2011-11-17 15:02:49 PST
Created attachment 575304 [details] [diff] [review]
Patch v2
Comment 4 Mozilla RelEng Bot 2011-11-17 19:30:30 PST
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
Comment 5 Justin Lebar (not reading bugmail) 2011-11-21 06:53:54 PST
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.
Comment 6 Ed Morley [:emorley] 2011-11-21 19:11:34 PST
https://hg.mozilla.org/mozilla-central/rev/7653280aa252
Comment 7 Justin Lebar (not reading bugmail) 2011-11-21 19:28:29 PST
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.
Comment 8 Steven Michaud [:smichaud] (Retired) 2011-12-08 08:13:25 PST
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 :-)
Comment 9 Emanuel Hoogeveen [:ehoogeveen] 2011-12-08 08:54:53 PST
(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.
Comment 10 Steven Michaud [:smichaud] (Retired) 2011-12-08 13:34:32 PST
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 :-)
Comment 11 Justin Lebar (not reading bugmail) 2011-12-08 15:21:19 PST
> 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.
Comment 12 Steven Michaud [:smichaud] (Retired) 2011-12-08 16:16:38 PST
> Let's check a few days before Dec. 20, at the latest?

Sounds fine to me.  I'm not on vacation until Dec 23rd.
Comment 13 Justin Lebar (not reading bugmail) 2011-12-15 12:11:50 PST
How's this look now?
Comment 14 Steven Michaud [:smichaud] (Retired) 2011-12-15 13:18:35 PST
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.
Comment 15 Steven Michaud [:smichaud] (Retired) 2011-12-22 08:34:32 PST
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.
Comment 16 Justin Lebar (not reading bugmail) 2011-12-22 08:52:36 PST
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+.
Comment 17 Alex Keybl [:akeybl] 2012-01-09 11:17:49 PST
Tracking for FF11 to back out after 11.0beta1 at the latest.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-01 13:18:33 PST
qa+ to track for QA during Firefox 11 Beta cycle -- however no explicit "fix verification" is needed at this point.
Comment 20 Mihaela Velimiroviciu (:mihaelav) 2012-02-28 07:24:37 PST
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.
Comment 21 Justin Lebar (not reading bugmail) 2012-03-05 11:09:46 PST
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.

Note You need to log in before you can comment on or make changes to this bug.