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

Status: VERIFIED FIXED

(Core :: Memory Allocator, defect)

Description

[Justin Lebar (not reading bugmail)]

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)]

Attachment: WIP v1

Comment 2

[Mozilla RelEng Bot]

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/[e-mail]-a025661fb603

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 4

[Mozilla RelEng Bot]

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/[e-mail]-70887ea960e2

Comment 5

[Justin Lebar (not reading bugmail)]

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]]

https://hg.mozilla.org/mozilla-central/rev/7653280aa252

Comment 7

[Justin Lebar (not reading bugmail)]

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)]

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]]

(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)]

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)]

> 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)]

> 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)]

How's this look now?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

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)]

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)]

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]]

Tracking for FF11 to back out after 11.0beta1 at the latest.

Comment 18

[u279076]

qa+ to track for QA during Firefox 11 Beta cycle -- however no explicit "fix verification" is needed at this point.

Comment 19

[Mihaela Velimiroviciu (:mihaelav)]

https://crash-stats.mozilla.com/query/query?product=Firefox&version=Firefox%3A11.0b2&version=Firefox%3A11.0b1&platform=mac&range_value=1&range_unit=weeks&date=02%2F14%2F2012+07%3A41%3A10&query_search=signature&query_type=exact&query=&reason=&build_id=&process_type=any&hang_type=any&do_query=1

It seems that there are no crashes with the isalloc_validate signature in the past 1 week on Mac betas

Comment 20

[Mihaela Velimiroviciu (:mihaelav)]

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)]

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.