1373586 - Assertion in LineBreakBefore due to ruby-related textrun failure

Status: REOPENED

(Core :: Layout: Text and Fonts, defect, P3)

Description

[Ekanan Ketunuti]

This bug was filed from the Socorro interface and is 
report bp-02ab3f9f-87a6-41d4-ad2b-6d5010170616.
=============================================================

STR:
1. load https://nanake.bitbucket.io/tests/wagahaiwanekodearu.html

Bisect says

The first bad revision is:
@  changeset:   352910:8d6560b363d2
|  user:        Kan-Ru Chen <[e-mail]>
~  date:        Mon Oct 24 22:00:03 2016 +0800
   files:       layout/generic/nsTextFrameUtils.cpp
   description:
   Bug 1081858 - Part 1. Fix aText off-by-one indexing. r=jfkthame

   MozReview-Commit-ID: 6LAlEntU6C7

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

I can reproduce this. It looks like nsTextFrame::EnsureTextRun cannot construct a text run so we got a nullptr when trying to use mTextRun later.

Might be related to bug 1371351 which has similar symptom that we were unable to build text run and rebuild it over and over again.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

I tried to run this with rr. It seems in BuildTextRunsScanner::BuildTextRunForFrames we always ended up with mSkipIncompleteTextRuns=true so textRun is set to nullptr.

TextContainsLineBreakerWhiteSpace looks suspicious because it only checks nsLineBreaker::IsSpace(aChar) || aChar == 0x0A;

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

I have a patch for bug 1369985 which should also fix this.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: simplified testcase

This is the simplified testcase from the original page, which the new crashtest being added in bug 1369985 would base on.

I found it pretty hard to further simplify this testcase, and TBH it is not completely clear why rbc frame would fail to get the text run of its children only under these specific conditions.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Hmmm, it seems I can still reproduce this with my patch in bug 1369985 on Windows at least, on top of the current m-c.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

But the failure becomes intermittent :/

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

There seems to be a different crash involves now... Although the final result is the same that the ruby frame fails to get text run. We probably should add a null check there anyway...

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

OK, so I guess another issue here is similar to that of bug 1369985, but probably related to whitespace trimming. I can reduce the testcase to another form that the crash happens within two paragraphs, and the stripped whitespace is at a line break. But this is not reliably reproducible, and I have no idea why...

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: another simplified testcase

It doesn't reproduce reliably. The crash can happen after several refresh.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

jfkthame, any thought? I guess some of callsites of IsTrimmableSpace may be suspicious... But I don't quite have direction where to look.

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> There seems to be a different crash involves now... Although the final
> result is the same that the ruby frame fails to get text run. We probably
> should add a null check there anyway...

Yes, I think we should always null-check the result of GetTextRun(), as there may be edge cases where EnsureTextRun fails for some reason. IIRC most call-sites have such a check already, but it's missing here.

Then it would be interesting to see if this can be converted into a testcase that doesn't crash, but shows incorrect rendering due to the failure to create the textrun.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> jfkthame, any thought? I guess some of callsites of IsTrimmableSpace may be
> suspicious... But I don't quite have direction where to look.

Offhand I'm not sure either, but leaving needinfo? for now as a reminder to have a look at this.

Comment 12

[Ekanan Ketunuti]

bp-dbf12fbf-1762-4383-b640-f05810170620 
The matter's not resolved. The culprit's still at large.

It crashes both link in comment 0 and attachment 8879432 [details].
Bug 1369985 seems to fix only the crashtest in attachment 8878858 [details].

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

attachment 8879432 [details] is the second testcase extracted from the large page mentioned in comment 0.

There might even be other patterns involve the link in comment 0 after the second testcase gets fixed. No one knows.

We probably should just add the null check here, optionally with a MOZ_ASSERT, so that we can catch such text run failure in debug build, but keep release build robust.

Comment 14

[Bob Clary [:bc] (inactive)]

Bughunter also reproduced this on Windows and Linux:

https://bug1373586.bmoattachments.org/attachment.cgi?id=8878858
bp-2b8f6ae5-dd25-4e78-9363-73efe1170621

https://nanake.bitbucket.io/tests/wagahaiwanekodearu.html
bp-e4df5da1-e06b-4824-9b96-228811170621

Comment 16

[Jonathan Kew [:jfkthame]]

Is this still happening on Nightly builds? I don't see any recent occurrences in crash-stats, afaict.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

The crash itself has been protected by bug 1374625 so there certainly no longer exists any new crashes because of this. However, if you open the second attached testcase in a debug build, it should still assert at the same place where it fails to build the text run.

Comment 18

[Jonathan Kew [:jfkthame]]

Given that this no longer represents a crash risk in release builds (thanks to bug 1374625), downgrading from P2/critical to P3/normal. It's still a bug we should try to diagnose and fix, however.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

See comment 18.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

How can I stop bugbug from closing it?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Yeah, bugbug shouldn't close bugs, specially with test-cases.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

This is an autonag rule. No crash => closed. And I disagree with you... I don't see the point of keeping this kind of bug open.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

The underlying issue is not fixed, and it is still something we may want to investigate. I guess I can just remove the crash signature so that the bot wouldn't close it based on that.

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

And probably the keyword.