1110181 - Hang/crash with writing-mode: vertical and text-indent with ch

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Simon Montagu :smontagu]

Attachment: Testcase (crashes!)

The attached testcase hangs briefly then crashes with 

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_PROTECTION_FAILURE at 0x00007fff5f3fffe8

The stack trace shows a loop around nsRuleNode::GetStyleText, ComputeTextData, SetCoord, CalcLength, GetMetricsFor, and WritingMode ctor

Comment 1

[Simon Montagu :smontagu]

Attachment: Top of stacktrace

Comment 2

[Jonathan Kew [:jfkthame]]

Ugh. Yeah, this crashes because we get infinite recursion when nsRuleNode::ComputeTextData wants to set a value that requires accessing the font metrics; but to do that, GetMetricsFor wants to set up a WritingMode, which depends on text-orientation; this comes from context's StyleText(), and getting that makes us recursively call ComputeTextData......

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Move mTextOrientation to the nsStyleVisibility struct to avoid potential recursive dependency in nsStyleText

This fixes the crash in my local build, as the use of the 'ch' unit from within ComputeTextData no longer causes recursion when we try to set up the WritingMode value to get font metrics.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8535045 [details] [diff] [review]
Move mTextOrientation to the nsStyleVisibility struct to avoid potential recursive dependency in nsStyleText

r=dbaron

Though it seems a little odd the way some style structs are passed as parameters to nsLayoutUtils::GetTextRunFlagsForStyle and others aren't...

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #4)
> Though it seems a little odd the way some style structs are passed as
> parameters to nsLayoutUtils::GetTextRunFlagsForStyle and others aren't...

Yes, I wondered whether we should do something to make this more consistent. This was intended as a minimal patch to fix the crash here, but we could do some cleanup there; we could also revert to passing an nsStyleVisibility to the WritingMode constructor (instead of an nsStyleContext), to make it clear that it doesn't depend on any other structs.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Followup to make WritingMode constructor take only an nsStyleVisibility, to ensure it cannot depend on other style structs in the context

Now that WritingMode() construction doesn't depend on any other style structs, I'd be inclined to do something along these lines, to make this more explicit - wdyt?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8535075 [details] [diff] [review]
Followup to make WritingMode constructor take only an nsStyleVisibility, to ensure it cannot depend on other style structs in the context

This seems a little annoying at the caller-side; I was thinking about another approach related to having explicit struct dependencies a la
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/a8e3ad9bfb5a/struct-computation-dependencies
and then adding a mechanism to assert that those are followed during computation.  The latter mechanism (which I haven't written yet) might be a cleaner substitute for this, perhaps?

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #7)
> Comment on attachment 8535075 [details] [diff] [review]
> Followup to make WritingMode constructor take only an nsStyleVisibility, to
> ensure it cannot depend on other style structs in the context
> 
> This seems a little annoying at the caller-side; I was thinking about
> another approach related to having explicit struct dependencies a la
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> a8e3ad9bfb5a/struct-computation-dependencies
> and then adding a mechanism to assert that those are followed during
> computation.  The latter mechanism (which I haven't written yet) might be a
> cleaner substitute for this, perhaps?

Yes, I assumed you meant a general style-struct-dependency mechanism; I think that would be a good thing, in principle, and should prevent future problems like this. So perhaps there's no need to worry about the specific WritingMode constructor case here, once we've fixed the current bug.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8535075 [details] [diff] [review]
Followup to make WritingMode constructor take only an nsStyleVisibility, to ensure it cannot depend on other style structs in the context

No real need to do this at this point.

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb8a621a5dd

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/4bb8a621a5dd