Closed
Bug 1332704
Opened 8 years ago
Closed 8 years ago
figure out what's going on with our sheet/document pointer in devtools tests
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(2 files)
2.05 KB,
patch
|
heycam
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.17 KB,
patch
|
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This is a followup to bug 851892. I disabled most of Rule::IsKnownLive because it runs into this problem on ASAN tests:
[task 2017-01-20T18:31:45.989011Z] 18:31:45 INFO - TEST-START | devtools/client/inspector/rules/test/browser_rules_add-property-cancel_02.js
[task 2017-01-20T18:31:54.798625Z] 18:31:54 INFO - =================================================================
[task 2017-01-20T18:31:54.800655Z] 18:31:54 INFO - ==1390==ERROR: AddressSanitizer: heap-use-after-free on address 0x61d00027813c at pc 0x7f8c84cc503f bp 0x7fffd38f42f0 sp 0x7fffd38f42e8
[task 2017-01-20T18:31:54.801677Z] 18:31:54 INFO - READ of size 4 at 0x61d00027813c thread T0 (Web Content)
[task 2017-01-20T18:31:56.496413Z] 18:31:56 INFO - #0 0x7f8c84cc503e in GetMarkedCCGeneration /home/worker/workspace/build/src/dom/base/nsIDocument.h:1936:12
[task 2017-01-20T18:31:56.496824Z] 18:31:56 INFO - #1 0x7f8c84cc503e in IsKnownLive /home/worker/workspace/build/src/layout/style/nsCSSRules.cpp:96
[task 2017-01-20T18:31:56.498766Z] 18:31:56 INFO - #2 0x7f8c84cc503e in mozilla::css::Rule::cycleCollection::CanSkipReal(void*, bool) /home/worker/workspace/build/src/layout/style/nsCSSRules.cpp:100
which suggests that the document pointer is dead for reasons that are unclear.
Unsurprisingly it got killed at:
[task 2017-01-20T18:31:56.878709Z] 18:31:56 INFO - #1 0x7f8c7e554404 in SnowWhiteKiller::~SnowWhiteKiller() /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2664:9
[task 2017-01-20T18:31:56.880034Z] 18:31:56 INFO - #2 0x7f8c7e553ff6 in nsCycleCollector::FreeSnowWhite(bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2839:3
[task 2017-01-20T18:31:56.897832Z] 18:31:56 INFO - #3 0x7f8c7fe5989e in AsyncFreeSnowWhite::Run() /home/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp:145:34
There's a similar thing after
[task 2017-01-20T18:33:54.569018Z] 18:33:54 INFO - TEST-START | devtools/client/inspector/rules/test/browser_rules_add-property-svg.js
![]() |
Assignee | |
Comment 1•8 years ago
|
||
Attachment #8828966 -
Flags: review?(cam)
![]() |
Assignee | |
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Updated•8 years ago
|
Attachment #8828966 -
Flags: review?(cam) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f11f6a536718
Have ~nsDocument actually clear the self-pointer from _all_ its sheets, and reenable Rule::IsKnownLive bits that depends on that pointer being managed correctly. r=heycam
![]() |
Assignee | |
Comment 3•8 years ago
|
||
I guess it might be a good idea to take this on branches too, just for stability reasons
![]() |
Assignee | |
Comment 4•8 years ago
|
||
Comment on attachment 8829468 [details] [diff] [review]
Branch version
Approval Request Comment
[Feature/Bug causing the regression]: Long long time ago, when additional sheets
were added.
[User impact if declined]: No known impact, but possible stability issue in
case some code tries to examine the document pointer on these sheets. These
sheets are NOT exposed to script, and I'm not aware of specific code that
examines the owning document of these sheets on branches, so there is no
known security issue here at this time.
[Is this code covered by automated tests?]: Sort of, in that we destroy
documents all the time.
[Has the fix been verified in Nightly?]: On try, at least.
[Needs manual test from QE? If yes, steps to reproduce]:
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: I don't think so.
[Why is the change risky/not risky?]: It's just nulling out backpointers that we
should have nulled out all along. Either no one is dereferencing them, and
then there's no effect, or someone is, and they will now at least get null
instead of garbage.
[String changes made/needed]: None.
Attachment #8829468 -
Flags: approval-mozilla-beta?
Attachment #8829468 -
Flags: approval-mozilla-aurora?
![]() |
Assignee | |
Comment 5•8 years ago
|
||
Also, if by the time this is being approved aurora is 53, not 52, then we will want to backport the "trunk" version to there instead...
Comment 6•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 7•8 years ago
|
||
Comment on attachment 8829468 [details] [diff] [review]
Branch version
clear back pointers from stylesheets to nsDocument on destruction, for beta52
Clearing aurora flag on this patch per comment 5.
Attachment #8829468 -
Flags: approval-mozilla-beta?
Attachment #8829468 -
Flags: approval-mozilla-beta+
Attachment #8829468 -
Flags: approval-mozilla-aurora?
Comment 8•8 years ago
|
||
Comment on attachment 8828966 [details] [diff] [review]
Have ~nsDocument actually clear the self-pointer from _all_ its sheets, and reenable Rule::IsKnownLive bits that depends on that pointer being managed correctly
clear back pointers from stylesheets to nsDocument on destruction, for aurora53
Moving aurora approval from the other patch.
Attachment #8828966 -
Flags: approval-mozilla-aurora+
Comment 9•8 years ago
|
||
bugherder uplift |
Comment 10•8 years ago
|
||
bugherder uplift |
status-firefox52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•