1276536 - Callers of CreateReferenceRenderingContext() cannot handle it returning null

Status: REOPENED

(Core :: Graphics, defect, P3)

Description

[Nicholas Nethercote [inactive]]

In theory, CreateReferenceRenderingContext() never returns null, viz:

> /**
>  * Get a reference rendering context. This is a context that should not
>  * be rendered to, but is suitable for measuring text and performing
>  * other non-rendering operations. Guaranteed to return non-null.
>  */
> virtual already_AddRefed<gfxContext> CreateReferenceRenderingContext() = 0;

But in practice this is false. PresShell::CreateReferenceRenderingContext() is the only implementation of that method and it has two null-returning paths.

This is a problem because all the callers of this function assume non-nullness, e.g. we have lots of code like this:

> nsRenderingContext rc( 
>   PresContext()->PresShell()->CreateReferenceRenderingContext());

This can create an nsRenderingContext with a null |mThebes|, which will cause null derefs (see the "These accessors will never return null" comment within nsRenderingContext).

Similarly, a few places do this:

> RefPtr<gfxContext> ctx =
>   aTextFrame->PresContext()->PresShell()->CreateReferenceRenderingContext();
> RefPtr<DrawTarget> dt = ctx->GetDrawTarget();

jwatt, any suggestions how to deal with this? There are lots of places that directly or indirectly assume that CreateReferenceRenderingContext() returns non-null. Do we just need to check for failure at every one of them?

Comment 1

[Jonathan Watt [:jwatt]]

This method was made infallible in bug 996351, which has follow-up discussion pointing to bug 1168934. Seems like this is a duplicate of that latter bug.

Comment 2

[Jonathan Watt [:jwatt]]

(There is currently some sort of server side error preventing me marking this as a duplicate.)

Comment 4

[Jonathan Watt [:jwatt]]

I should also say:

Technically nsDeviceContext::CreateRenderingContext can also fail if gfx::Factory::CreateRecordingDrawTarget fails, but that's a debug code path.

Also gfxContext::ForDrawTarget(gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget()) should be infallible. I'll file a follow-up for that.

Comment 5

[Nicholas Nethercote [inactive]]

It's very closely related to bug 1168934 but not an exact duplicate, AFAICT.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> gfxContext::ForDrawTarget(gfxPlatform::GetPlatform()-
> >ScreenReferenceDrawTarget()) should be infallible. I'll file a follow-up
> for that.

bug 1276810

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> It's very closely related to bug 1168934 but not an exact duplicate, AFAICT.

Are you seeing something other than the things I noted in comment 4? If not, it seems like a duplicate to me.

Comment 8

[Jonathan Watt [:jwatt]]

Well, I guess I'm making the leap to assuming that Jeff and Bas being in favor of returning a dummy DrawTarget means CreateReferenceRenderingContext will automatically become infallible again. Let's mark this as depending on that bug then.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

There are very few, if any, truly infallible calls in graphics, and we're in the process of making even fewer of them be that way.  More to the point, making the callers be ready for the failure.  It's an imperfect work in progress, and this would be moving us in the wrong direction of the work done in the past year and a half.

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to Milan Sreckovic [:milan] from comment #9)
> There are very few, if any, truly infallible calls in graphics, and we're in
> the process of making even fewer of them be that way.  More to the point,
> making the callers be ready for the failure.  It's an imperfect work in
> progress, and this would be moving us in the wrong direction of the work
> done in the past year and a half.

So there's two ways to handle this bug:

- Change CreateReferenceRenderingContext() so it never returns null.

- Change the callers of CreateReferenceRenderingContext() so they handle null.

I'm fine with the latter if that makes more sense. It just needs to be done :)

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Agreed, but since I don't think we can do the former anyway, we're only left with the latter.  Unless we count crashing as not returning null, that is.  If crashes counts, then we can start by adding a crash for all return paths with the "callers of this function can't handle null return" type of message.
Given that we have ~10 files that call this function, we could have them all individually crash/assert when they get the result they can't handle.  It is a bit like pulling on a thread, so we may want to do it in stages.