Closed
Bug 1332704
Opened 7 years ago
Closed 7 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•7 years ago
|
||
Attachment #8828966 -
Flags: review?(cam)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Updated•7 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•7 years ago
|
||
I guess it might be a good idea to take this on branches too, just for stability reasons
Assignee | ||
Comment 4•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f11f6a536718
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 7•7 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•7 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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/56500dbe56f3
Comment 10•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e2e9026ac248
status-firefox52:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•