[css-grid] Crash on null pointer [@ InvalidArrayIndex_CRASH | nsComputedDOMStyle::GetGridTemplateColumnsRows]

RESOLVED FIXED in Firefox 59

Status

()

P3
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: truber, Assigned: bradwerth)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla59
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

Attachments

(4 attachments)

Created attachment 8851441 [details]
testcase.html

The attached testcase crashes in mozilla-central rev f5e214144799.

==21875==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x00000051ad76 bp 0x7ffde339a5d0 sp 0x7ffde339a460 T0)
==21875==The signal is caused by a WRITE memory access.
==21875==Hint: address points to the zero page.
    #0 0x51ad75 in MOZ_CrashPrintf /home/worker/workspace/build/src/mfbt/Assertions.cpp:63:3
    #1 0x7f863169f24f in InvalidArrayIndex_CRASH(unsigned long, unsigned long) /home/worker/workspace/build/src/xpcom/ds/nsTArray.cpp:26:3
    #2 0x7f8637b1ff13 in ElementAt /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1054:7
    #3 0x7f8637b1ff13 in operator[] /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1083
    #4 0x7f8637b1ff13 in nsComputedDOMStyle::GetGridTemplateColumnsRows(nsStyleGridTemplate const&, mozilla::ComputedGridTrackInfo const*) /home/worker/workspace/build/src/layout/style/nsComputedDOMS
tyle.cpp:2877
    #5 0x7f8637b21573 in nsComputedDOMStyle::DoGetGridTemplateColumns() /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:3066:10
    #6 0x7f8637b02b8e in nsComputedDOMStyle::GetPropertyCSSValue(nsAString const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:991:11
    #7 0x7f8637b00d78 in nsComputedDOMStyle::GetPropertyValue(nsAString const&, nsAString&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:404:26
    #8 0x7f8634d58fc1 in GetPropertyValue /home/worker/workspace/build/src/layout/style/nsICSSDeclaration.h:129:10
    #9 0x7f8634d58fc1 in mozilla::dom::CSSStyleDeclarationBinding::getPropertyValue(JSContext*, JS::Handle<JSObject*>, nsICSSDeclaration*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src
/obj-firefox/dom/bindings/CSSStyleDeclarationBinding.cpp:165
Flags: in-testsuite?
status-firefox55: affected → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox58: --- → affected
Priority: -- → P3
fyi, bughunter reproduces this practically everywhere but only with the attached test case. I haven't seen it in the wild yet.
INFO: Last good revision: 679118259e91f40d4a8f968f03ec4cff066cdb5b (2016-07-10)
INFO: First bad revision: 214884d507ee369c1cf14edb26527c4f9a97bf48 (2016-07-11)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=679118259e91f40d4a8f968f03ec4cff066cdb5b&tochange=214884d507ee369c1cf14edb26527c4f9a97bf48
Blocks: 1241932
Has Regression Range: --- → yes
status-firefox57: affected → wontfix
status-firefox58: affected → fix-optional
status-firefox-esr52: --- → wontfix
(Assignee)

Comment 3

a year ago
Surely due to something I touched in Bug 1241932.
Assignee: nobody → bwerth
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8928042 - Flags: review?(mats)
Attachment #8928043 - Flags: review?(mats)

Comment 7

a year ago
mozreview-review
Comment on attachment 8928042 [details]
Bug 1350780 Part 1: Change nsComputedDOMStyle::DoGetGridTemplate{Columns|Rows} to take grid templates from mInnerFrame.

https://reviewboard.mozilla.org/r/199268/#review204528

After reading nsComputedDOMStyle::UpdateCurrentStyleSources, this chunk in particular:
https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/layout/style/nsComputedDOMStyle.cpp#997-1007
I think using mContent->GetPrimaryFrame() here is wrong.
We should just use mInnerFrame.

The ::-moz-selection case should return 'none' because this pseudo-element
doesn't have a box, and 'grid-template-columns' isn't inherited.
Attachment #8928042 - Flags: review?(mats) → review-
Created attachment 8928222 [details] [diff] [review]
fix?

So, this should fix the crash.  I haven't tested more than that though...

Could you also add an identical test to the one you have but with
s/::-moz-selection/::before/ 
The result for that should also be 'none', for the same reason.

And a third test, also for ::before, but with an added style like so:

<style>
div::before {
  content: "";
  display: grid;
  grid-template-columns: 40px;
}
</style>

In this case the result should be "40px".
It might be worth adding a fourth test for the ::before case where the pseudo
doesn't have a frame, like so:

<style>
div::before {
  display: grid;
  grid-template-columns: 40px;
}
</style>

The result should be "40px".  If you can figure out a way to differentiate
the results for test three/four would be good too, to ensure we're actually
using the result from the frame in test three, not just the computed value.
Using 'auto' instead of 40px perhaps?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
Comment on attachment 8928042 [details]
Bug 1350780 Part 1: Change nsComputedDOMStyle::DoGetGridTemplate{Columns|Rows} to take grid templates from mInnerFrame.

https://reviewboard.mozilla.org/r/199268/#review204660
Attachment #8928042 - Flags: review?(mats) → review+

Comment 15

a year ago
mozreview-review
Comment on attachment 8928043 [details]
Bug 1350780 Part 2: Add a test of getComputedStyle with pseudo element styling on an unflowed display:grid element.

https://reviewboard.mozilla.org/r/199270/#review204662

Thanks!
Attachment #8928043 - Flags: review?(mats) → review+

Comment 16

a year ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5dc9b4abb08
Part 1: Change nsComputedDOMStyle::DoGetGridTemplate{Columns|Rows} to take grid templates from mInnerFrame. r=mats
https://hg.mozilla.org/integration/autoland/rev/cd4e8b693249
Part 2: Add a test of getComputedStyle with pseudo element styling on an unflowed display:grid element. r=mats
https://hg.mozilla.org/mozilla-central/rev/c5dc9b4abb08
https://hg.mozilla.org/mozilla-central/rev/cd4e8b693249
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is this worth backporting to Beta or can it ride the 59 train?
Flags: needinfo?(bwerth)
Flags: in-testsuite?
Flags: in-testsuite+
(Assignee)

Comment 19

a year ago
This can wait for 59. It's a marginal case and according to Comment 1 hasn't been spotted in the wild.
Flags: needinfo?(bwerth)
status-firefox58: fix-optional → wontfix
(In reply to Brad Werth [:bradwerth] from comment #19)
> This can wait for 59. It's a marginal case and according to Comment 1 hasn't
> been spotted in the wild.

Should we reconsider based on bug 1421592?
Flags: needinfo?(bwerth)
(Assignee)

Comment 21

a year ago
(In reply to Julien Cristau [:jcristau] from comment #20)
> (In reply to Brad Werth [:bradwerth] from comment #19)
> > This can wait for 59. It's a marginal case and according to Comment 1 hasn't
> > been spotted in the wild.
> 
> Should we reconsider based on bug 1421592?

I don't think so. I can't reproduce Bug 1421592, and I attempted to reproduce it again after unwinding the changes in this bug. There is something going on with Bug 1421592 (I can still get memory leaks), but it doesn't appear to be fixed by this patch.
Flags: needinfo?(bwerth)
You need to log in before you can comment on or make changes to this bug.