1228670 - Crash [@ GetMaxOptionBSize] — <optgroup style="display: list-item; list-style: inside;"></optgroup>

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Debug:
  [@ GetMaxOptionBSize]
  Stack attached

Nightly:
  [@ GetMaxOptionBSize]
  bp-e49b0664-a7f5-4651-9c07-0995a2151127

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=22f39dd335cc5b62373e74ebbf07accd28208281&tochange=090f71020a97

Triggered by: Bug 1212688

Comment 3

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

[Tracking Requested - why for this release]: Shouldn't be shipping crash regressions

Comment 4

[Mats Palmgren (inactive)]

Attachment: frame tree

The <optgroup> has a child nsBulletFrame which returns null from the GetContentInsertionFrame() call.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

Perhaps this function should also skip nested <optgroup>s? (as bug 1228876)

Comment 6

[Mats Palmgren (inactive)]

Attachment: crashtest

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8694213 [details] [diff] [review]
fix

r=me, but please add a comment explaining why "frame" might be null here (if "option" is an anonymous leaf frame of some sort, sounds like).

Skip nested optgroups in what sense?  There should be no such thing in the frame tree; the frame constructor enforces that.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 44 since it's a crash.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4f9aad0350
https://hg.mozilla.org/integration/mozilla-inbound/rev/18149c916fbf

Comment 10

[Mats Palmgren (inactive)]

> There should be no such thing in the frame tree;

Ah, good point.  Thanks.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8694213 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: 1212688
[User impact if declined]: crash
[Describe test coverage new/current, TreeHerder]: have crashtest
[Risks and why]: zero risk, just an added null-pointer check
[String/UUID change made/needed]: none

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d4f9aad0350
https://hg.mozilla.org/mozilla-central/rev/18149c916fbf

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8694213 [details] [diff] [review]
fix

Crash fixes are always good. Aurora44+

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jesse, could you please verify that the crash is gone? Thanks!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ca1c9d211cd
https://hg.mozilla.org/releases/mozilla-aurora/rev/1708e3abd9b0

Comment 16

[Jesse Ruderman]

Yep, this is fixed for me on mozilla-central. The patch landed with a crashtest, btw :)

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6ca1c9d211cd
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1708e3abd9b0

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Jesse Ruderman from comment #16)
> Yep, this is fixed for me on mozilla-central. The patch landed with a
> crashtest, btw :)

Thanks for the verification and crashtests are awesome :)