Closed Bug 1332704 Opened 3 years ago Closed 3 years ago

figure out what's going on with our sheet/document pointer in devtools tests

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

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 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
Attached patch Branch versionSplinter Review
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.
Attachment #8829468 - Flags: approval-mozilla-beta?
Attachment #8829468 - Flags: approval-mozilla-aurora?
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...
https://hg.mozilla.org/mozilla-central/rev/f11f6a536718
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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 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.