Closed Bug 1332704 Opened 4 years ago Closed 4 years ago
figure out what's going on with our sheet/document pointer in devtools tests
Have ~nsDocument actually clear the self-pointer from _all_ its sheets, and reenable Rule::IsKnownLive bits that depends on that pointer being managed correctly
2.05 KB, patch
|Details | Diff | Splinter Review|
1.17 KB, patch
|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: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8828966 - Flags: review?(cam) → review+
Pushed by firstname.lastname@example.org: 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
I guess it might be a good idea to take this on branches too, just for stability reasons
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.
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 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.
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+
You need to log in before you can comment on or make changes to this bug.