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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Have ~nsDocument actually clear the self-pointer from _all_ its sheets, and reenable Rule::IsKnownLive bits that depends on that pointer being managed correctly

Comment 2

[Pulsebot]

Pushed by [e-mail]:
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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch version

I guess it might be a good idea to take this on branches too, just for stability reasons

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f11f6a536718

Comment 7

[Julien Cristau [:jcristau] (away, back August 16)]

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 8

[Julien Cristau [:jcristau] (away, back August 16)]

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.

Comment 9

[[Back August 16] Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/56500dbe56f3

Comment 10

[[Back August 16] Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2e9026ac248