1296410 - NULL pointer read [@xul!nsLineLayout::ReflowFrame]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

I am only seeing this on windows at the moment. It doesn't seem to be consistent but the attached test case can reproduce the issue. You may need to close the browser between attempts.

FAULTING_SOURCE_FILE:  c:\mozilla-central\layout\generic\nslinelayout.cpp

FAULTING_SOURCE_LINE_NUMBER:  945

FAULTING_SOURCE_CODE:  
   941: 
   942:   if (!isText) {
   943:     aFrame->Reflow(mPresContext, reflowOutput, *reflowInputHolder, aReflowStatus);
   944:   } else {
>  945:     static_cast<nsTextFrame*>(aFrame)->
   946:       ReflowText(*this, availableSpaceOnLine,
   947:                  psd->mReflowInput->mRenderingContext->GetDrawTarget(),
   948:                  reflowOutput, aReflowStatus);
   949:   }
   950: 

xul!nsLineLayout::ReflowFrame+0x337
xul!nsBlockFrame::ReflowInlineFrame+0x33
xul!nsBlockFrame::DoReflowInlineFrames+0x163
xul!nsBlockFrame::ReflowInlineFrames+0xfa
xul!nsBlockFrame::ReflowLine+0x55
xul!nsBlockFrame::ReflowDirtyLines+0x422
xul!nsBlockFrame::Reflow+0x335
xul!nsBlockReflowContext::ReflowBlock+0x1c7
xul!nsBlockFrame::ReflowBlockFrame+0x50b
xul!nsBlockFrame::ReflowLine+0x44
xul!nsBlockFrame::ReflowDirtyLines+0x422
xul!nsBlockFrame::Reflow+0x335
xul!nsContainerFrame::ReflowChild+0x4d
xul!nsCanvasFrame::Reflow+0x1e2
...
See attached log

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

I should also mention this is a fuzz blocker on windows.

Comment 2

[Ethan Lin[:ethlin]]

I can reproduce this problem with layer.acceleration.disabled: true.

Comment 3

[Ethan Lin[:ethlin]]

I tested it locally and I noted that the measureText gets zero width while the scale is very large, comparing to the normal case, measureText works fine. But I'm not sure why it causes ReflowFrame problem.
nical, do you have any idea?

Comment 4

[Nicolas Silva [:nical]]

(In reply to Ethan Lin[:ethlin] from comment #3)
> nical, do you have any idea?

No idea off hands, sorry.

Comment 5

[Ethan Lin[:ethlin]]

When setting a large scale and calling MeasureText, cairo fails at 'GetCharWidth' and then gecko gets an invalid drawtarget. I'll see how to fix it.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: fix crash

The problem is when we set a large scale to cairo, the logical size of font will be very large. Cairo will fail at GetCharWidth32 [1] and then the status of cairo becomes abnormal. The abnormal backend status causes this crash.

GetCharWidth32 in cairo is added in bug 96041 [2]. I think when GetCharWidth32 fails, we just need to reset the width of char and don't need to change the status.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-win32-font.c#975
[2] https://hg.mozilla.org/mozilla-central/rev/81b50b2f2ece

Comment 7

[Ethan Lin[:ethlin]]

Attachment: add crashtest

Add crashtest for this bug.

Comment 8

[Markus Stange [:mstange]]

Do you know why the cairo error status causes us to crash?

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8803799 [details] [diff] [review]
fix crash

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

This patch seems ok on its own though.

But there are other places in this code that return errors, we should not be crashing on those either...

Comment 10

[Ethan Lin[:ethlin]]

(In reply to Markus Stange [:mstange] from comment #8)
> Do you know why the cairo error status causes us to crash?

We call 'PresShell::CreateReferenceRenderingContext()' [1] to create gfxContext and then enter 'gfxContext::CreateOrNull' [2]. The error status of cairo makes the return value of 'aTarget->IsValid()' is false. So we get null gfxContext. When trying to get the drawtarget from the gfxContext [3], we'll have crash.
I tried to check if gfxContext is nullptr at [3], but gecko will still crash at another place.


[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#2974
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxContext.cpp?q=%2Bfunction%3A%22gfxContext%3A%3ACreateOrNull%28mozilla%3A%3Agfx%3A%3ADrawTarget+%2A%2C+const+mozilla%3A%3Agfx%3A%3APoint+%26%29%22&redirect_type=single#88
[3] https://dxr.mozilla.org/mozilla-central/source/gfx/src/nsRenderingContext.h?q=%2Bfunction%3AnsRenderingContext%3A%3AGetDrawTarget%28%29&redirect_type=single#35

Comment 11

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

Yes, sometimes the error handling/fault tolerance is a deep stack, and fixing one place pops up the bug/crash in another place.  We should still fix the places we find.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8803799 [details] [diff] [review]
fix crash

Jeff had some more thoughts on this.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Ethan, can you track down how the status is getting into the cairo context? It seems reasonable for us to mark the scaled font as bad, but we should avoid having bad scaled fonts make our cairo_t go bad. There are other places where this function can return a bad status so it would be good to make sure we don't run into the same problem those ways.

Comment 14

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Ethan, are you still working on this bug?

Comment 16

[Ethan Lin[:ethlin]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Ethan, are you still working on this bug?

No, I just tried and I cannot reproduce this problem on current release and nightly.

Comment 17

[Ethan Lin[:ethlin]]

I think we still can land the crashtest.

Comment 18

[Ethan Lin[:ethlin]]

try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38204dfc9442b89725f248bda3ca9ffda03c7f9f

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eece8c1550bb
Add crashtest for the bug about NULL pointer read when opening a canvas html. r=mstange

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eece8c1550bb

Comment 21

[Ryan VanderMeulen [:RyanVM]]

mozregression says this got fixed somewhere in https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a551f534773cf2d6933f78ce7d82a7a33a99643e&tochange=c724ae8bb7b867b6bc65efa1049c0322457ff4b0.

Given that we're not really fuzzing Beta with Grizzly, I'm not going to bother backporting the crashtest.