1439524 - InspectorUtils.getUsedFontFaces may crash when a textrun maps multiple content nodes

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement)

Description

[Jonathan Kew [:jfkthame]]

See bug 1435989 comment 17 and following.

When the textrun maps multiple content nodes, and we try to map textrun-based offsets of the glyph run back to underlying content offsets, we may get a result that extends far beyond the current frame's content node. We need to clamp offsets appropriately.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Clamp glyph-run offsets appropriately for the current frame's content to avoid potential failure to create the Range

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Add testcase for getUsedFontFaces that exercises multiple DOM nodes and whitespace collapsing

This adds a testcase that crashes before the patch above is applied.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8952352 [details] [diff] [review]
Add testcase for getUsedFontFaces that exercises multiple DOM nodes and whitespace collapsing

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

r=me

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8952351 [details] [diff] [review]
Clamp glyph-run offsets appropriately for the current frame's content to avoid potential failure to create the Range

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

::: layout/base/nsLayoutUtils.cpp
@@ +8126,5 @@
> +                                                    aRange.start));
> +      int32_t end =
> +        aSkipIter.ConvertSkippedToOriginal(std::min(glyphRuns.GetStringEnd(),
> +                                                    aRange.end));
> +      end = std::min(end, contentLimit);

Could you add a code-comment here to hint at why we need these three new std::min, std::max, std::min clamps?  (and hinting at the distinction between the first pair vs. the third)

Comment 5

[Daniel Holbert [:dholbert]]

In particular, I'm confused at why we need the first pair of clamps (clamping to aRange.start/end), given that the thing we're clamping (from glyphRuns) was *initialized* using |aRange|.  It seems like it should be smart enough to already be clamping to that range?

I think it does clamp to that range internally in the NextRun() impl, here:
> mStringStart = std::max(mStartOffset, mGlyphRun->mCharacterOffset);
> ...
> mStringEnd = std::min(mEndOffset, last);
https://searchfox.org/mozilla-central/source/gfx/thebes/gfxTextRun.cpp#78-82

Here, mStartOffset is aRange.start, and mEndOffset is aRange.end.  And mStringStart / mStringEnd are the things returned from glyphRuns.GetStringStart() & glyphRuns.GetStringEnd().  So the additional aRange clamping seems redundant.


I suspect the final clamp ("end = std::min(end, contentLimit);") is the one that really matters here.  That one could still benefit from an explanatory code-comment, mentioning the relevant invariants that we do/don't have here and hence why we need to clamp.  (And I think the other two std::min/max can go away?)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Clamp glyph-run offsets appropriately for the current frame's content to avoid potential failure to create the Range

I believe you're right, those clampings should be redundant given how GlyphRunIterator works; removed them, and added a comment to explain the important one. One other minor tweak, too: skip returning any ranges that turn out to be completely empty.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8952471 [details] [diff] [review]
Clamp glyph-run offsets appropriately for the current frame's content to avoid potential failure to create the Range

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

r=me, one nit:

::: layout/base/nsLayoutUtils.cpp
@@ +8154,5 @@
>  nsLayoutUtils::GetFontFacesForText(nsIFrame* aFrame,
>                                     int32_t aStartOffset,
>                                     int32_t aEndOffset,
>                                     bool aFollowContinuations,
> +                                   nsLayoutUtils::UsedFontFaceTable& aFontFaces,

This patch is adding a nsLayoutUtils:: prefix here in the GetFontFacesForText() definition -- but I don't think that's necessary. This is a nsLayoutUtils:: method call, so I think nsLayoutUtils-scoped types are automatically in-scope for the args (which is how we were able to compile before this patch, I imagine).

Or: if I'm missing something and we really do need the prefix here, then I expect we also need to add this prefix to the parameter for the GetFontFacesForFrames() definition, too:
https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/layout/base/nsLayoutUtils.cpp#8084

Comment 8

[Jonathan Kew [:jfkthame]]

Ah, that's a leftover; at one point I was going to refactor things so this would become a method on nsTextFrame, but then I backtracked on that. I'll remove the redundant prefix.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c755e956453d
Clamp glyph-run offsets appropriately for the current frame's content to avoid potential failure to create the Range. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f797d4d040a
Add testcase for getUsedFontFaces that exercises multiple DOM nodes and whitespace collapsing. r=dholbert

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c755e956453d
https://hg.mozilla.org/mozilla-central/rev/5f797d4d040a