gfxContext::ForDrawTarget{,WithTransform}() complicate things, leading to lots of redundant checks and also some missing ones. Removing them improves things. More detail in the forthcoming patch.

gfxContext::ForDrawTarget() is a wrapper for gfxContext's constructor. It checks if the passed-in DrawTarget is null or invalid, and returns null in that case. Otherwise, it just invokes the constructor. Most of the call sites already check for nullness and validity before calling, in which case the checks inside ForDrawTarget() are unnecessary. Some call sites instead check whether ForDrawTarget() succeeded. Many call sites unnecessarily do checks both before and after. However, some ForDrawTarget() call sites fail to check either before or after. So we typically have redundant checks or insufficient checks. This patch improves things. - It makes gfxContext's constructor infallible -- it now aborts (even in release builds) if the DrawTarget is null or invalid. This forces all the checking to take place beforehand. - It removes gfxContext::ForDrawTarget() and replaces calls to it with direct |new gfxContext()| calls, which are more clearly infallible. This also makes it more obvious that object creation is occurring. - It removes the assertions and checks that gfxContext creation succeeds, which are now vacuously true. - It adds nullness and validity checks to all the call sites that were lacking them. - It changes one gfxCriticalError() call in a check to gfxDevCrash(LogReason::InvalidContext) for consistency. The patch does likewise ForDrawTargetWithTransform(). That function did a tiny bit more than just call the gfxContext constructor, but it has only three call sites so the code duplication is negligible, and the change is worth it for consistency and clarity at the call sites. (In fact, the one in nsDisplayList.cpp did unnecessary extra work.)

jwatt, can you double-check the removed ForDrawTargetWithTransform() call in nsDisplayList.cpp? There's a SetMatrix() call immediately afterwards, so I think we don't need to inline the one from the call itself at the call site -- i.e. there's no point calling SetMatrix() twice in a row with different matrices, right?

Jeff suggested doing this in bug 1259513 comment 2 (the bug that introduced gfxContext::ForDrawTarget) and Milan/Jeff/Bas/Lee decided to go with the fallible helper. I'm technically not a gfx peer, so I don't feel I can go against that decision without their approval.

Comment on attachment 8758088 [details] [diff] [review] Remove gfxContext::ForDrawTarget{,WithTransform}() Review of attachment 8758088 [details] [diff] [review]: ----------------------------------------------------------------- As tempting it is to believe that we can keep this going, the past is showing that people just assume that "new ..." will succeed. It will get through the code reviews, it will bite us in the future. This is basically undoing the work in bug 1259513 which landed in 48. We had this discussion in there, let's not reopen it and churn the code again. On a side note, we MOZ_CRASH in graphics, rather than MOZ_RELEASE_ASSERT.

> As tempting it is to believe that we can keep this going, the past is > showing that people just assume that "new ..." will succeed. It will get > through the code reviews, it will bite us in the future. This is basically > undoing the work in bug 1259513 which landed in 48. We had this discussion > in there, let's not reopen it and churn the code again. IMO the work in bug 1259513 should be undone: - Currently there are two ways to check. Beforehand, with |!dt || !dt->IsValid()|, or afterwards, by checking ForDrawTarget()'s return value. Most places check beforehand. Some check after. Many check both. Some *check neither* -- there are still bugs here. - gfxContext::ForDrawTarget() is a poor name for a factory method; it's completely non-obvious that this does gfxContext creation. - My patch is a net reduction of 85 lines of code. I'd be ok with changing my patch to keep the factory method and changing its name. Something like gfxContext::CreateFromValidDT() would make clear both (a) that it's doing construction, and (b) that the DT must be valid. I'm not ok with leaving the code as-is, because it's buggy. There are still missing checks. > On a side note, we MOZ_CRASH in graphics, rather than MOZ_RELEASE_ASSERT. Ok. You know they're almost identical, right? They both call MOZ_CRASH_ANNOTATE and MOZ_REALLY_CRASH.

(In reply to Nicholas Nethercote [:njn] from comment #5) > I'd be ok with changing my patch to keep the factory method and changing its > name. Something like gfxContext::CreateFromValidDT() would make clear both > (a) that it's doing construction, and (b) that the DT must be valid. I think this would be a good improvement. However, "CreateFromValidDT" makes it sound like the callers may be responsible for doing the validity check and the factory method may crash if the DT is invalid. If the factory method is going to check for validity and possibly return null instead of crashing (presumably preferable to meet project uptime's goal) then perhaps call the method MaybeCreateForDT? If we really want to force callers to check the return value we could of course return Maybe<gfxContext>, but that seems like overkill if "Maybe" is in the name of the method. ;) Presumably you'd be happy to change ForDrawTarget's name, Milan?

(In reply to Nicholas Nethercote [:njn] from comment #5) > > As tempting it is to believe that we can keep this going, the past is > > showing that people just assume that "new ..." will succeed. It will get > > through the code reviews, it will bite us in the future. This is basically > > undoing the work in bug 1259513 which landed in 48. We had this discussion > > in there, let's not reopen it and churn the code again. > > IMO the work in bug 1259513 should be undone: > > - Currently there are two ways to check. Beforehand, with |!dt || > !dt->IsValid()|, or afterwards, by checking ForDrawTarget()'s return value. > Most places check beforehand. Some check after. Many check both. Some *check > neither* -- there are still bugs here. Checking twice is better than not checking. There should be no places where things are worse than before. So, whatever bugs are there, we're no worse off than before bug 1259513, and in a number of places better off. > > - gfxContext::ForDrawTarget() is a poor name for a factory method; it's > completely non-obvious that this does gfxContext creation. Don't care about names, rename away. > > - My patch is a net reduction of 85 lines of code. Not worried about that. It isn't like it's complex code. We can probably get the same reduction in loc by removing the before checks in the places that already check after. > > I'd be ok with changing my patch to keep the factory method and changing its > name. Something like gfxContext::CreateFromValidDT() would make clear both > (a) that it's doing construction, and (b) that the DT must be valid. Right. Don't care about the names. > > I'm not ok with leaving the code as-is, because it's buggy. There are still > missing checks. Where there are bugs, getting rid of this wrapper/checker method alone would in no way remove those bugs, and keeping this approach in no way precludes us from applying the same fixes. Having "new" fail/crash for any reason other than an out of memory situation is not acceptable. It's a public method, it's your job to deal with any kind of input you're given, crashing is wrong. If the caller crashes because they gave you the bad input, so you gave them empty output, that's fair - they made a mistake, they should crash. > > > > On a side note, we MOZ_CRASH in graphics, rather than MOZ_RELEASE_ASSERT. > > Ok. You know they're almost identical, right? They both call > MOZ_CRASH_ANNOTATE and MOZ_REALLY_CRASH. Sure thing - as long as they both show up in the moz crash reason in the crash reports, and they're used with the comment, no big deal. Since they are equivalent, and we have 15 files with 25 uses of the release assert, compared to 50 files and 135 uses of the moz crash, consistency also makes things easier. When I search through the source code to see if all moz crashes have GFX: prefix, it's nice I don't have to worry about the release asserts as well.

(In reply to Jonathan Watt [:jwatt] from comment #6) > (In reply to Nicholas Nethercote [:njn] from comment #5) > > I'd be ok with changing my patch to keep the factory method and changing its > > name. Something like gfxContext::CreateFromValidDT() would make clear both > > (a) that it's doing construction, and (b) that the DT must be valid. > > I think this would be a good improvement. However, "CreateFromValidDT" makes > it sound like the callers may be responsible for doing the validity check > and the factory method may crash if the DT is invalid. If the factory method > is going to check for validity and possibly return null instead of crashing > (presumably preferable to meet project uptime's goal) then perhaps call the > method MaybeCreateForDT? > > If we really want to force callers to check the return value we could of > course return Maybe<gfxContext>, but that seems like overkill if "Maybe" is > in the name of the method. ;) > > Presumably you'd be happy to change ForDrawTarget's name, Milan? Rename away :) As long as the one with the wrong data pays for trying to use it without checking, I'm good. I don't like being able to crash any public method by calling it with certain parameters. We certainly have a lot of examples of that in the code today, but that doesn't make it right.

(In reply to Jonathan Watt [:jwatt] from comment #6) > > I think this would be a good improvement. However, "CreateFromValidDT" makes > it sound like the callers may be responsible for doing the validity check > and the factory method may crash if the DT is invalid. If the factory method > is going to check for validity and possibly return null instead of crashing > (presumably preferable to meet project uptime's goal) then perhaps call the > method MaybeCreateForDT? I chose that name assuming that it *would* crash if passed an invalid DT. We have differing priorities here: - I want to pave the cowpaths: the vast majority of callers already do a |!dt || !dt->IsValid()| check anyway, so it makes sense to me that we force all callers to do that. - I also want to crash immediately and reliably and obviously if something goes wrong, rather than (probably) crashing later and less obviously. - Milan doesn't want a public method that's crashable on certain inputs. We can't satisfy all three of these :(

Force none of the callers to do a check before, and just have them check the return value. If they already check before, it means they can handle the return value of null. Nobody needs to crash. If they haven't been checking, and the function that makes the call has other places that return "failures", they should return the failure when they get back null. If they haven't been checking, and the function isn't set up to fail, MOZ_CRASH. All three satisfied.

Heh, well "Force none of the callers to do a check before" goes against the first of Nick's three points, but yeah, I'd agree that's what we should do, and it aligns with my proposal to name the factory method MaybeCreateForDT and have *it* do the checks and maybe return null.

I went through all the ForDrawTarget{,WithTransform}() calls to see how the checking is handled. There is quite a variety. Checks before and after: - null+invalid check before (no logging) + MOZ_ASSERT after: 22 - null+invalid check before (w/gfxDevCrash) + MOZ_ASSERT after: 4 - null+invalid check before (w/NS_ERROR) + MOZ_ASSERT after: 3 - null+invalid check before (w/gfxWarning) + MOZ_ASSERT after: 3 - null+invalid check before (w/ALOG_BRIDGE) + MOZ_ASSERT after: 2 - null+invalid check before (w/NS_WARNING) + MOZ_ASSERT after: 1 - null+invalid check before (w/gfxCriticalNote) + MOZ_ASSERT after: 1 - redundant null+invalid MOZ_ASSERT before + MOZ_ASSERT after: 1 - null check before (w/NS_ERROR) + null check after (w/NS_ERROR): 1 Checks after - null check after (w/gfxDevCrash): 9 - null check after (w/gfxCriticalError): 1 - null check after (MOZ_RELEASE_ASSERT): 1 BAD: Missing validity check: - null check before (w/NS_ABORT_OOM): 1 BAD: Missing both checks: - in browser code: 1 - in test code: 2 UNCERTAIN: - unsure about before checks: 1 - unsure about before checks + MOZ_ASSERT after: 1

The new names gfxContext::MaybeCreate{,WithTransform}() better communicate that these functions (a) do object creation, and (b) are fallible.

This patch modifies all the places where checks were definitely missing or possibly missing. There are some change I'm unsure about, which I've marked with "njn:" comments. I'm asking milan and jwatt both for feedback because this patch straddles gfx/, layout/ and dom/.

> Checks before and after: > - null+invalid check before (no logging) + MOZ_ASSERT after: 22 > - null+invalid check before (w/gfxDevCrash) + MOZ_ASSERT after: 4 > - null+invalid check before (w/NS_ERROR) + MOZ_ASSERT after: 3 > - null+invalid check before (w/gfxWarning) + MOZ_ASSERT after: 3 > - null+invalid check before (w/ALOG_BRIDGE) + MOZ_ASSERT after: 2 > - null+invalid check before (w/NS_WARNING) + MOZ_ASSERT after: 1 > - null+invalid check before (w/gfxCriticalNote) + MOZ_ASSERT after: 1 > - redundant null+invalid MOZ_ASSERT before + MOZ_ASSERT after: 1 > - null check before (w/NS_ERROR) + null check after (w/NS_ERROR): 1 I forgot to mention that the factory methods themselves also call gfxCriticalNote() on failure, so in several of the above cases the failure will be logged twice.

Comment on attachment 8759986 [details] [diff] [review] (part 1) - Rename gfxContext::ForDrawTarget{,WithTransform}() Review of attachment 8759986 [details] [diff] [review]: ----------------------------------------------------------------- Love it.

Can I suggest calling the factory method CreateOrNull. The "OrNull" pattern is pretty well established in our code as a way of indicating that null may be returned, whereas the "Maybe*" pattern seems to be used for methods that return a bool to indicate whether or not some _optional_ (as opposed to failure) action was taken.

Comment on attachment 8759987 [details] [diff] [review] (part 2) - Fix up checks for calls to gfxContext::MaybeCreate{,WithTransform}() Review of attachment 8759987 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +3603,5 @@ > // not render well via the code below. > if (mOp == CanvasRenderingContext2D::TextDrawOperation::FILL && > mState->StyleIsColor(CanvasRenderingContext2D::Style::FILL)) { > + // njn: I can't tell if the mCtx->EnsureTarget() call above guarantees > + // non-nullness and validity of mCtx->mTarget The way the code is today, it'll crash if thebes is nullptr, so I'd put a MOZ_CRASH like thing in there. @@ +4992,5 @@ > GlobalAlpha() == 1.0f && > UsedOperation() == CompositionOp::OP_OVER) > { > + // njn: SupportsAzureContentForDrawTarget() does a null check, not sure if > + // we have a validity check It does not check, but it really should. Checking for IsValid is a fairly new addition to the code, and we haven't caught up in most of the places. Generally, either when we find a crash, or when we're changing the code in the same area anyway, we make this change. In this case, I'd just add IsValid() check at the top of SupportsAzureContentForDrawTarget() since "we're there." ::: gfx/layers/basic/BasicLayerManager.cpp @@ -930,5 @@ > > PaintSelfOrChildren(paintLayerContext, groupTarget); > > - // Temporary fast fix for bug 725886 > - // Revert these changes when 725886 is ready Is bug 725886 ready? Seems to be a duplicate of bug 715433 which is still open. ::: gfx/tests/gtest/gfxFontSelectionTest.cpp @@ +194,5 @@ > SurfaceFormat::B8G8R8X8); > RefPtr<gfxContext> ctx = gfxContext::MaybeCreate(drawTarget); > + if (!ctx) { > + MOZ_CRASH("gfxContext creation failed"); > + } This is a test, so may as well crash and then decide, but this would be a new crash - ctx.forget() wouldn't crash before, even if RefPtr points to a nullptr, right? ::: layout/svg/nsSVGIntegrationUtils.cpp @@ +469,5 @@ > SurfaceFormat::A8) > : ctx.GetDrawTarget()->CreateSimilarDrawTarget(maskSurfaceRect.Size(), > SurfaceFormat::A8); > + if (!maskDT || !maskDT->IsValid()) { > + // njn: do I need to do something with aOutMaskSurface, aOutMaskTransform? There is already an assumption in the code that aOutMaskSurface comes in null, and if we return with it null, we know bad things happened and deal with it accordingly, so it looks like returning is enough.

For MaybeCreateWithTransform I'd suggest calling it CreateOrNullPreserveTransform. (CreateOrNullWithTransform and CreateWithTransformOrNull would be confusing. "WithTransform" is bad phrasing anyway since it sound like a transform should be passed, so it was never a great name.)

(In reply to Milan Sreckovic [:milan] from comment #18) > The way the code is today, it'll crash if thebes is nullptr, so I'd put a > MOZ_CRASH like thing in there. If we're about to do a straight de-reference of a pointer we don't typically add release checks just before to cause a crash if it's null, do we? We're going to crash non-exploitably anyway. (If we're going to pass the pointer out or turn it into a reference and reference some offset into it I can see the point.) If we do want to add such checks I'd think we should use MOZ_RELEASE_ASSERT for brevity rather than an |if| statement wrapping a MOZ_CRASH (one line vs. three). > ::: layout/svg/nsSVGIntegrationUtils.cpp > @@ +469,5 @@ > > SurfaceFormat::A8) > > : ctx.GetDrawTarget()->CreateSimilarDrawTarget(maskSurfaceRect.Size(), > > SurfaceFormat::A8); > > + if (!maskDT || !maskDT->IsValid()) { > > + // njn: do I need to do something with aOutMaskSurface, aOutMaskTransform? > > There is already an assumption in the code that aOutMaskSurface comes in > null, and if we return with it null, we know bad things happened and deal > with it accordingly, so it looks like returning is enough. I agree with that.

(In reply to Jonathan Watt [:jwatt] from comment #20) > (In reply to Milan Sreckovic [:milan] from comment #18) > > The way the code is today, it'll crash if thebes is nullptr, so I'd put a > > MOZ_CRASH like thing in there. > > If we're about to do a straight de-reference of a pointer we don't typically > add release checks just before to cause a crash if it's null, do we? Sure, no need for an explicit crash.

(In reply to Nicholas Nethercote [:njn] from comment #12) > I went through all the ForDrawTarget{,WithTransform}() calls to see how the > checking is handled. There is quite a variety. > > Checks before and after: > - null+invalid check before (no logging) + MOZ_ASSERT after: 22 ... I consider MOZ_ASSERT a debugging tool once a bug with an STR is found. It's a no-op as far as the users are concerned, so anything that just has a MOZ_ASSERT as a way to "guard" against badness is inadequate. With that out of the way, I guess the logging part is optional - some of the callers are probably quite used to things being invalid at that point and don't consider it an out of the ordinary situation. Overall, we can probably over rotate on this bug. Let's make things better, but if we're looking for "perfect code", we may be looking for a while.

I changed the names to Create{,PreservingTransform}OrNull(), much as jwatt suggested.

Comment on attachment 8760509 [details] [diff] [review] (part 1) - Rename gfxContext::ForDrawTarget{,WithTransform}() Review of attachment 8760509 [details] [diff] [review]: ----------------------------------------------------------------- Carrying over r+.

(In reply to Milan Sreckovic [:milan] from comment #21) > (In reply to Jonathan Watt [:jwatt] from comment #20) > > If we're about to do a straight de-reference of a pointer we don't typically > > add release checks just before to cause a crash if it's null, do we? > > Sure, no need for an explicit crash. Okay, cool. The same idea applies to the MOZ_ASSERTs added in nsDisplayList.cpp and nsSVGIntegrationUtils.cpp, since we'll crash via a null-dereference there too.

> ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +3603,5 @@ > > // not render well via the code below. > > if (mOp == CanvasRenderingContext2D::TextDrawOperation::FILL && > > mState->StyleIsColor(CanvasRenderingContext2D::Style::FILL)) { > > + // njn: I can't tell if the mCtx->EnsureTarget() call above guarantees > > + // non-nullness and validity of mCtx->mTarget > > The way the code is today, it'll crash if thebes is nullptr, so I'd put a > MOZ_CRASH like thing in there. I ended up just putting an TODO comment saying that it's unclear. At least that will help people looking at this code in the future. (Also I have an idea for a better fix for all of this code that I will attempt on a plane to London.) > @@ +4992,5 @@ > > GlobalAlpha() == 1.0f && > > UsedOperation() == CompositionOp::OP_OVER) > > { > > + // njn: SupportsAzureContentForDrawTarget() does a null check, not sure if > > + // we have a validity check > > It does not check, but it really should. I added an IsValid() check to SupportsAzureContentForDrawTarget(). > ::: gfx/layers/basic/BasicLayerManager.cpp > @@ -930,5 @@ > > > > PaintSelfOrChildren(paintLayerContext, groupTarget); > > > > - // Temporary fast fix for bug 725886 > > - // Revert these changes when 725886 is ready > > Is bug 725886 ready? Seems to be a duplicate of bug 715433 which is still > open. I removed the |untransformedDT && untransformedDT->IsValid()| assertion because exactly the same check is done 12 lines earlier. I thought the comment referred to the assertion but after doing some bug archaeology (bug 780792) I'm not sure if that comment is still relevant, and which lines of code it's referring to... I'll just put the comment back. > ::: gfx/tests/gtest/gfxFontSelectionTest.cpp > @@ +194,5 @@ > > SurfaceFormat::B8G8R8X8); > > RefPtr<gfxContext> ctx = gfxContext::MaybeCreate(drawTarget); > > + if (!ctx) { > > + MOZ_CRASH("gfxContext creation failed"); > > + } > > This is a test, so may as well crash and then decide, but this would be a > new crash - ctx.forget() wouldn't crash before, even if RefPtr points to a > nullptr, right? ctx.forget() won't crash, but subsequent code that uses ctx would. (Well, turns out this test is disabled anyway, hrmph, but the principal holds.)

> The same idea applies to the MOZ_ASSERTs added in nsDisplayList.cpp and > nsSVGIntegrationUtils.cpp, since we'll crash via a null-dereference there > too. We have 22 places where we already have such an assert (see comment 15), it's the most common pattern which is why I used it.

Comment on attachment 8760530 [details] [diff] [review] (part 2) - Fix up checks for calls to gfxContext::Create{,PreservingTransform}OrNull() Review of attachment 8760530 [details] [diff] [review]: ----------------------------------------------------------------- \o/

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5336c82efe871a75ebc04e199b27db00fd96ad9 Bug 1276824 (part 1) - Rename gfxContext::ForDrawTarget{,WithTransform}(). r=milan. https://hg.mozilla.org/integration/mozilla-inbound/rev/9de4baab6d239b5ddb5e208d4d739eafc57067b7 Bug 1276824 (part 2) - Fix up checks for calls to gfxContext::Create{,PreservingTransform}OrNull(). r=milan.

https://hg.mozilla.org/mozilla-central/rev/b5336c82efe8 https://hg.mozilla.org/mozilla-central/rev/9de4baab6d23

I think bug 1259513 is the correct one?