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

VERIFIED FIXED in Firefox 11

Status

()

Core
Memory Allocator
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

({verified-beta})

unspecified
mozilla11
x86
Mac OS X
verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11+ verified)

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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

6 years ago
Blocks: 414946
(Assignee)

Updated

6 years ago
Blocks: 694896
(Assignee)

Comment 1

6 years ago
Created attachment 575004 [details] [diff] [review]
WIP v1
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug

Comment 2

6 years ago
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)

Comment 3

6 years ago
Created attachment 575304 [details] [diff] [review]
Patch v2
(Assignee)

Updated

6 years ago
Attachment #575004 - Attachment is obsolete: true

Comment 4

6 years ago
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

6 years ago
Attachment #575304 - Attachment description: WIP v2 → Patch v2
Attachment #575304 - Flags: review?(khuey)
Attachment #575304 - Flags: review?(khuey) → review+
(Assignee)

Comment 5

6 years ago
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

6 years ago
https://hg.mozilla.org/mozilla-central/rev/7653280aa252
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Comment 7

6 years ago
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 :-)
(Assignee)

Comment 11

6 years ago
> 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.
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 16

6 years ago
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+]
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
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

5 years ago
Blocks: 731696
(Assignee)

Comment 21

5 years ago
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.