Closed Bug 1350780 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: truber, Assigned: bradwerth)

References

Details

(Keywords: crash, testcase)

Attachments

(4 files)

testcase.html
408 bytes, text/html
Details
Bug 1350780 Part 1: Change nsComputedDOMStyle::DoGetGridTemplate{Columns|Rows} to take grid templates from mInnerFrame.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1350780 Part 2: Add a test of getComputedStyle with pseudo element styling on an unflowed display:grid element.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
fix?
1.39 KB, patch
Details | Diff | Splinter Review
Attached file 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?
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
Surely due to something I touched in Bug 1241932.
Assignee: nobody → bwerth
Attachment #8928042 - Flags: review?(mats)
Attachment #8928043 - Flags: review?(mats)
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-
Attached patch fix? — — Splinter Review 
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 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 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+
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
Closed: 7 years 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+
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)
(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)
(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.

Attachment

General

Creator:
Created:
Updated:
Size: