Closed Bug 1124892 Opened 9 years ago Closed 9 years ago

Large OOMs with only xul.dll frames in 36 and higher

Categories

(Toolkit :: Crash Reporting, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- unaffected
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed

People

(Reporter: kairo, Assigned: away)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

xul.dll has grown by 8 megs between 34 and 36, mostly due to the DLL merges in bug 609976 and bug 922912. We likely need to update ReserveBreakpadVM's kReserveSize.

(I bet OOM|small is equally affected, but since Socorro doesn't put stack frames in those signatures, we don't notice)
Bug 1028972 comment 15: "This ought to buy us a year and change. Here's hoping we'll revisit this for unified xul before then." Well, at least that came true :-)
[Tracking Requested - why for this release]: This causes a lot of useless crash signatures on 36+
Increase by 10MB to cover recent growth plus a little future breathing room.

(For anyone following along, this patch is not intended to fix the crashes, but it should turn the "xul.dll@0x1234" signatures into something more actionable)
Attachment #8553365 - Flags: review?(benjamin)
Some of the signatures I checked had lots of youtube URLs. That makes the importance of trying to figure these crashes out all the more important for MSE. Adding Anthony and Sheila to the bug as we will need to discuss this next week.
Attachment #8553365 - Flags: review?(benjamin) → review+
Important crash, tracking.
David, can you please request uplift to aurora and beta for this ASAP? As you know, we have those crashes on beta that we need data for and then need to react to that, so the sooner we can get this there, the better.
Flags: needinfo?(dmajor)
Comment on attachment 8553365 [details] [diff] [review]
Adjust Breakpad reservation for xul.dll inflation

[Triage Comment]
I am going to bypass the procedure to make sure we have it in beta 4.
David, hope you don't mind!
Attachment #8553365 - Flags: approval-mozilla-beta+
Attachment #8553365 - Flags: approval-mozilla-aurora+
Given that it landed on the branches, clearing the ni?
Flags: needinfo?(dmajor)
https://hg.mozilla.org/mozilla-central/rev/8aa2406244bc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Agreed re the uplifts, thanks for jumping in!
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #13)
> From what I see, it's substantially lower volume, but we still have reports
> like this in 36.0b4 even with this patch:

Yes, I was just going to mention that as well, http://bit.ly/1tyKWex shows almost all youtube.com URLs which concerns me a bit since this patch made into that build ID.
David: Is there anything in particular we could tweak to get the remaining signatures to show more information? The concern from the MSE side would be that many URLs show youtube, and it would great to find out what the root cause of those remaining crashes is. Thanks.
Flags: needinfo?(dmajor)
The previous patch really should have worked, but I guess we can try increasing the value even higher.

In the meantime, WinDbg can make sense of the dump files if you already have a PDB cached for that xul version. If you have a list of the top N bogus signatures that you care about, I can go and open bugs with the correct stacks.
Flags: needinfo?(dmajor) → needinfo?(mozillamarcia.knous)
Attachment #8556794 - Flags: review?(benjamin)
> http://bit.ly/1tyKWex 
That one looks like it's bug 1102642.
Attachment #8556794 - Flags: review?(benjamin) → review+
It looks as if it may be too late to get this in B5 since I already see build candidates - ni on Sylvestre to see the possibilities.

Sylvestre we see crashes in the latest B4 explosive report that we would like to get a better stack for, so getting this patch into Beta 5 may be important for the MSE decision.
Flags: needinfo?(mozillamarcia.knous) → needinfo?(sledru)
Not sure we should postpone beta 5. Even if we start a build2 right now, we won't have the QE sign off from SV and QE before next Monday.
Beta 6 is going to build next Monday, should be released next Tuesday.
Flags: needinfo?(sledru)
Comment on attachment 8556794 [details] [diff] [review]
If at first you don't succeed, use more memory

Approval Request Comment
[Feature/regressing bug #]: xul.dll growth, mostly bug 609976 and bug 922912
[User impact if declined]: Unactionable crash reports
[Describe test coverage new/current, TreeHerder]: m-i is green
[Risks and why]: We will use more address space
[String/UUID change made/needed]: None
Attachment #8556794 - Flags: approval-mozilla-beta?
Attachment #8556794 - Flags: approval-mozilla-aurora?
Comment on attachment 8556794 [details] [diff] [review]
If at first you don't succeed, use more memory

I don't know if it is possible but maybe we should automatically detect if this needs to be increased.
Attachment #8556794 - Flags: approval-mozilla-beta?
Attachment #8556794 - Flags: approval-mozilla-beta+
Attachment #8556794 - Flags: approval-mozilla-aurora?
Attachment #8556794 - Flags: approval-mozilla-aurora+
I went ahead and pulled up the correct stacks for a few of these on 36b4: they led to bug 1128409, bug 1114976, bug 1081911.
Ryan could you help with the uplift of comment 23? (I'm surprised it didn't happen automagically. Were the scripts thrown off by the previous patch? Was there some marking I should have put on the bug?)
Flags: needinfo?(ryanvm)
(In reply to David Major [:dmajor] (UTC+13) from comment #26)
> Ryan could you help with the uplift of comment 23? (I'm surprised it didn't
> happen automagically. Were the scripts thrown off by the previous patch? Was
> there some marking I should have put on the bug?)

Yeah, the status flags should have been set back to affected when there was more follow-up work to land.
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.