Closed
Bug 1350780
Opened 8 years ago
Closed 7 years ago
[css-grid] Crash on null pointer [@ InvalidArrayIndex_CRASH | nsComputedDOMStyle::GetGridTemplateColumnsRows]
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: truber, Assigned: bradwerth)
References
Details
(Keywords: crash, testcase)
Attachments
(4 files)
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?
Updated•7 years ago
|
status-firefox56:
--- → wontfix
status-firefox57:
--- → affected
status-firefox58:
--- → affected
Priority: -- → P3
Comment 1•7 years ago
|
||
fyi, bughunter reproduces this practically everywhere but only with the attached test case. I haven't seen it in the wild yet.
Comment 2•7 years ago
|
||
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
Assignee | ||
Comment 3•7 years ago
|
||
Surely due to something I touched in Bug 1241932.
Assignee: nobody → bwerth
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8928042 -
Flags: review?(mats)
Attachment #8928043 -
Flags: review?(mats)
Assignee | ||
Comment 6•7 years ago
|
||
Comment 7•7 years 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-
Comment 8•7 years ago
|
||
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".
Comment 9•7 years ago
|
||
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) |
Assignee | ||
Comment 13•7 years ago
|
||
Comment 14•7 years 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•7 years 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•7 years 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
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c5dc9b4abb08
https://hg.mozilla.org/mozilla-central/rev/cd4e8b693249
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 18•7 years ago
|
||
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•7 years 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)
Updated•7 years ago
|
Comment 20•7 years ago
|
||
(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?
Updated•7 years ago
|
Flags: needinfo?(bwerth)
Assignee | ||
Comment 21•7 years 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.
Description
•