Closed Bug 1276810 Opened 3 years ago Closed 3 years ago

Make gfxPlatform::ScreenReferenceDrawTarget() infallible

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jwatt, Assigned: jwatt, NeedInfo)

Details

Attachments

(1 file)

Spinning this off from bug 1276536 comment 4.

gfxPlatform::ScreenReferenceDrawTarget is practically infallible, in the sense that it returns non-null, and if we check the cached object IsValid() then it really should be usable without any checks (which is what most code is already doing).
Attached patch patchSplinter Review
Attachment #8758067 - Flags: review?(bas)
Attachment #8758067 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/a42b3a900949
https://hg.mozilla.org/mozilla-central/rev/7edc972d4206
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8758067 [details] [diff] [review]
patch

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

Currently, the ScreenReferenceDrawTarget() won't return null, because we will crash in gfxPlatform, using MOZ_CRASH, before we let that happen.  A change like this though, introduces further strong coupling to this situation.  So, if we ever change that situation in one place (gfxPlatform), we will have to remember to change it in a number of other places.  It's situations like this, where people forgot to do this (despite the documentation and clearly defined what needs to happen) that was keeping the graphics dominating the top 20 crashes.  That's improved quite a bit in the past year and a half, and this is pushing us backwards.
This bug is probably misleadingly named. This patch pretty much just documented a pre-existing situation and made things consistent. 24 out of 25 callers were already assuming ScreenReferenceDrawTarget was infallible.

The only coupling that this patch introduces is the change in nsPresShell.cpp. Feel free to back out that specific part, r=me. As for the rest of it, I think if ScreenReferenceDrawTarget should not be treated as infallible that should be a separate bug. That would be a much wider change to change a pre-existing situation, and backing all of this bug's patch out would just get us back to an inconsistent state and do little to fix that.
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9575284041e
follow-up - Backout the nsPresShell.cpp change to reduce infallibility coupling. r=milan
(In reply to Jonathan Watt [:jwatt] from comment #6)
> Feel free to back out that specific part, r=me.

I took the liberty of backing that part out, r= you, Milan, to save you having to. Hopefully that addresses your concerns.
Flags: needinfo?(milan)
(In reply to Jonathan Watt [:jwatt] from comment #8)
> (In reply to Jonathan Watt [:jwatt] from comment #6)
> > Feel free to back out that specific part, r=me.
> 
> I took the liberty of backing that part out, r= you, Milan, to save you
> having to. Hopefully that addresses your concerns.

Thanks.

There are additional related fixes that are inspired by this patch, so I'll open a separate bug for that (e.g., we now check mScreenReferenceDrawTarget->IsValid(), so we should do the same a few lines above, and check mScreenReferenceDrawTarget->IsValid() as well.)

Also, where we removed if (!aTarget) return... check, we should either put it back, or put a MOZ_CRASH in there, otherwise if we ever return nullptr, we will crash a few lines down when dereferencing aTarget...
(In reply to Milan Sreckovic [:milan] from comment #10)
> Also, where we removed if (!aTarget) return... check, we should either put
> it back, or put a MOZ_CRASH in there, otherwise if we ever return nullptr,
> we will crash a few lines down when dereferencing aTarget...

If we're going to do something there then, as noted in comment 6, we should really do it at the other 24 callers of ScreenReferenceDrawTarget too.
Yes, let's do whatever we end up doing in another bug.  I'll put something in.
You need to log in before you can comment on or make changes to this bug.