Callers of CreateReferenceRenderingContext() cannot handle it returning null

REOPENED
Unassigned

Status

()

Core
Graphics
P3
normal
REOPENED
2 years ago
a year ago

People

(Reporter: njn, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox49 affected)

Details

(Whiteboard: [gfx-noted])

(Reporter)

Description

2 years ago
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?

Updated

2 years ago
Flags: needinfo?(jwatt)
Whiteboard: [gfx-noted]
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.
Flags: needinfo?(jwatt)
(There is currently some sort of server side error preventing me marking this as a duplicate.)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1168934
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.
(Reporter)

Comment 5

2 years ago
It's very closely related to bug 1168934 but not an exact duplicate, AFAICT.
(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
(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.
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.
Status: RESOLVED → REOPENED
Depends on: 1168934
Resolution: DUPLICATE → ---
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.
(Reporter)

Comment 10

2 years ago
(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 :)
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.
You need to log in before you can comment on or make changes to this bug.