Closed Bug 1219919 Opened 4 years ago Closed 4 years ago

Add suppressions for Windows-specific content process graphics leaks

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed
b2g-v2.5 --- fixed
b2g-master --- affected

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(3 files, 1 obsolete file)

See bug 1219916. I'm adding suppressions so Aurora isn't orange.
Comment on attachment 8680873 [details] [diff] [review]
Ignore some non-Nightly texture leaks.

Review of attachment 8680873 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine, but judging from bug 1219916, comment 0 it sounds like this might not be a stable number?

::: testing/mozbase/mozleak/mozleak/leaklog.py
@@ +75,5 @@
> +        'PTextureChild': 4,
> +        'SharedMemory': 4,
> +        'TextureChild': 4,
> +        'WeakReference<MessageListener>': 8,
> +    })

Is the amount constant during testing or should we add some padding?
Attachment #8680873 - Flags: review?(erahm) → review+
(In reply to Eric Rahm [:erahm] from comment #2)
> Seems fine, but judging from bug 1219916, comment 0 it sounds like this
> might not be a stable number?

Yeah, probably not, but I doubt anybody is landing new tests on Aurora so it will at least tide us over for 5 weeks.

> Is the amount constant during testing or should we add some padding?

That's a good question, but oddly enough it does seem to be stable:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

If it starts getting flaky and exceeding it sometimes I can bump it up more.
This is a test-only change so I think it can also be landed on Aurora without approval.
Keywords: checkin-needed
See Also: → 1232549
This is just an updated version of the patch for the leaks that show up on Aurora 45, so I'm just going to carry forward erahm's review.
Attachment #8701102 - Flags: review?(erahm)
Attachment 8701102 [details] [diff] needs to be landed on Aurora only.
Keywords: checkin-needed
Comment on attachment 8701102 [details] [diff] [review]
Ignore some non-Nightly texture leaks.

Review of attachment 8701102 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this already landed, just clearing the review.
Attachment #8701102 - Flags: review?(erahm) → review+
Oops, I meant to just r+ it myself.
FYI, I'm seeing some similar-looking leaks on an Aurora-as-Beta Try simulation push for Gecko 45.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ffa74919006&group_state=expanded&filter-searchStr=win%20debug%20e10s
These are actually just Windows-specific graphics leaks, but we were only seeing them on Aurora because leak logging was broken there. I'll update this to include more leaks that Bob's patches have revealed.

This also blocks removing the exit(0) for WinXP on Nightly (bug 1242119), because when you do that additional leaks show up. This does not require Bob's patches. I'm guessing that WinXP doesn't have the tighter sandbox we use on Win7 or something.
Blocks: 1219369, 1242119
Summary: Add suppressions for non-Nightly texture leaks. → Add suppressions for Windows-specific graphics leaks
Summary: Add suppressions for Windows-specific graphics leaks → Add suppressions for Windows-specific content process graphics leaks
Here's a new gigantic suppression list for Windows based on Bob's patch getting leak checking working.
This is a horribly long list, but it seems better to have the giant list than to not run leak logging on Windows content processes at all. The additional leaks are guarded behind mozinfo.isWin, which means they only apply to Windows. This function is only called for content processes, so it does not affect our leak checking otherwise.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdc3933abda0

There's a failure in e10s dt1, but I've added another SyncObject to the suppression list to account for that.
Attachment #8715017 - Flags: review?(erahm)
Comment on attachment 8715017 [details] [diff] [review]
Add suppressions for Windows-specific content process graphics leaks.

Review of attachment 8715017 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8715017 - Flags: review?(erahm) → review+
(In reply to Pulsebot from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3885c33f2013

I grafted this to Aurora too since it's effectively NPOTB and helps cut down on the Try uplift simulation noise. Also makes it easier to uplift bug 1219369 to Aurora as well.
https://hg.mozilla.org/releases/mozilla-aurora/rev/52d03258c744

Maybe we should consider this and bug 1219369 for Beta45 as well?
Flags: needinfo?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #14)

> This also blocks removing the exit(0) for WinXP on Nightly (bug 1242119),
> because when you do that additional leaks show up. This does not require
> Bob's patches. I'm guessing that WinXP doesn't have the tighter sandbox we
> use on Win7 or something.

WinXP doesn't have integrity levels, so the sandbox is pretty weak there at the moment ... only removing Admin rights and a couple of other things.
https://hg.mozilla.org/mozilla-central/rev/3885c33f2013
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> I grafted this to Aurora too since it's effectively NPOTB and helps cut down
> on the Try uplift simulation noise.

I don't understand what you mean here. Isn't mozilla-central-as-Aurora going to use the suppression file from m-c?

> Also makes it easier to uplift bug 1219369 to Aurora as well.
> https://hg.mozilla.org/releases/mozilla-aurora/rev/52d03258c744

KWierso already landed an earlier version of these suppressions on Aurora, in comment 13, so this new landing is essentially increasing the leak limit on Aurora. Please back one or the other out (the newer one is better, but it will less work to back out).

> Maybe we should consider this and bug 1219369 for Beta45 as well?

I'm not sure how useful that would be, as it isn't like this has resulted in any leaks being fixed, but I guess it wouldn't hurt. Really, there's a cluster of 5 or so bugs (bug 1091917 and everything blocking it) that are needed to make leak checking really work for Windows content processes.

(In reply to Bob Owen (:bobowen) from comment #20)
> WinXP doesn't have integrity levels, so the sandbox is pretty weak there at
> the moment ... only removing Admin rights and a couple of other things.

I figured that was what was happening. Thanks for confirming.
Flags: needinfo?(continuation) → needinfo?(ryanvm)
(In reply to Andrew McCreight [:mccr8] from comment #22)
> I don't understand what you mean here. Isn't mozilla-central-as-Aurora going
> to use the suppression file from m-c?

I was seeing issues on Aurora-as-Beta too.

> KWierso already landed an earlier version of these suppressions on Aurora,
> in comment 13, so this new landing is essentially increasing the leak limit
> on Aurora. Please back one or the other out (the newer one is better, but it
> will less work to back out).

Done.
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6605dddf40e

> I'm not sure how useful that would be, as it isn't like this has resulted in
> any leaks being fixed, but I guess it wouldn't hurt. Really, there's a
> cluster of 5 or so bugs (bug 1091917 and everything blocking it) that are
> needed to make leak checking really work for Windows content processes.

Doesn't sound worth it then, thanks.
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> I was seeing issues on Aurora-as-Beta too.

Does it look like those will be covered by the additional suppressions?

> Done.
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a6605dddf40e

Thanks!
(In reply to Andrew McCreight [:mccr8] from comment #24)
> Does it look like those will be covered by the additional suppressions?
> 

The Try push I kicked off last night says yes :). I'll let you know if anything changes after today's backout.
Attachment #8714983 - Attachment is obsolete: true
Duplicate of this bug: 1232549
You need to log in before you can comment on or make changes to this bug.