Closed Bug 1296410 Opened 4 years ago Closed 3 years ago

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


(Core :: Canvas: 2D, defect, P1, critical)




Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed


(Reporter: tsmith, Assigned: ethlin)


(Blocks 1 open bug)


(Keywords: crash, csectype-nullptr, testcase, Whiteboard: [fuzzblocker][gfx-noted])


(4 files)

Attached file 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


   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:   }

See attached log
Attached file test_case.html
I should also mention this is a fuzz blocker on windows.
Flags: in-testsuite?
I can reproduce this problem with layer.acceleration.disabled: true.
Whiteboard: [fuzzblocker]
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?
Flags: needinfo?(nical.bugzilla)
Whiteboard: [fuzzblocker] → [fuzzblocker][gfx-noted]
(In reply to Ethan Lin[:ethlin] from comment #3)
> nical, do you have any idea?

No idea off hands, sorry.
Flags: needinfo?(nical.bugzilla)
Priority: -- → P1
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.
Assignee: nobody → ethlin
Attached patch fix crashSplinter Review
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.

Attachment #8803799 - Flags: review?(mstange)
Attached patch add crashtestSplinter Review
Add crashtest for this bug.
Attachment #8803800 - Flags: review?(mstange)
Do you know why the cairo error status causes us to crash?
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...
Attachment #8803799 - Flags: review?(mstange) → review+
Attachment #8803800 - Flags: review?(mstange) → review+
(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.

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 on attachment 8803799 [details] [diff] [review]
fix crash

Jeff had some more thoughts on this.
Attachment #8803799 - Flags: review+ → review?(jmuizelaar)
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.
Ethan, are you still working on this bug?
Flags: needinfo?(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.
Flags: needinfo?(ethlin)
Attachment #8803799 - Flags: review?(jmuizelaar)
I think we still can land the crashtest.
Pushed by
Add crashtest for the bug about NULL pointer read when opening a canvas html. r=mstange
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: in-testsuite? → in-testsuite+
Keywords: stale-bug
mozregression says this got fixed somewhere in

Given that we're not really fuzzing Beta with Grizzly, I'm not going to bother backporting the crashtest.
You need to log in before you can comment on or make changes to this bug.