1374625 - Protect ruby from crash because of failing to build text run

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Most callsites of nsTextFrame::GetTextRun already have null check. We should add such check for ruby as well, because EnsureTextRun may fail in some cases (although we don't want it to ever fail).

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1374625 - Null-check result from GetTextRun in nsRubyBaseContainerFrame.

Review commit: https://reviewboard.mozilla.org/r/150830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150830/

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 8879538 [details]
Bug 1374625 - Null-check result from GetTextRun in nsRubyBaseContainerFrame.

https://reviewboard.mozilla.org/r/150830/#review155618

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72a0af4b7041
Null-check result from GetTextRun in nsRubyBaseContainerFrame. r=jfkthame

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/72a0af4b7041

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8879538 [details]
Bug 1374625 - Null-check result from GetTextRun in nsRubyBaseContainerFrame.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1081858
[User impact if declined]: May crash in some pages before we fix every cases of bug 1373586.
[Is this code covered by automated tests?]: No, because the added branch is just to improve robustness, and it is not supposed to happen at all.
[Has the fix been verified in Nightly?]: Just landed.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: Not risky at all.
[Why is the change risky/not risky?]: It just adds a null-check which would crash otherwise.
[String changes made/needed]: n/a


[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: May crash in some pages before we fix every cases of bug 1373586.
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): Not risky.
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 6

[Julien Cristau [:jcristau]]

Comment on attachment 8879538 [details]
Bug 1374625 - Null-check result from GetTextRun in nsRubyBaseContainerFrame.

null ptr check, beta55+

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aae1e9b911d8

Comment 8

[Cornel Ionce [:noni] [Hubs QA]]

Based on comment 5, this does not manual coverage. 
Updating the qe‑verify flag to reflect this.

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8879538 [details]
Bug 1374625 - Null-check result from GetTextRun in nsRubyBaseContainerFrame.

There is no signature associate with this. We can't tell how severe it is for users. Release54-.

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8879538 [details]
Bug 1374625 - Null-check result from GetTextRun in nsRubyBaseContainerFrame.

The same as comment #9. ESR52-.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Signature is the same as bug 1373586.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Given the signature, reconsider?

Comment 13

[Gerry Chang [:gchang]]

The volume of crashes in ESR52 is very low. It seems not a critical issue.