Bug 121892 - Tie the life of DocumentStyleSheetCollection and Document together
Summary: Tie the life of DocumentStyleSheetCollection and Document together
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-24 21:51 PDT by Benjamin Poulain
Modified: 2013-09-25 18:18 PDT (History)
15 users (show)

See Also:


Attachments
Patch (59.38 KB, patch)
2013-09-24 21:54 PDT, Benjamin Poulain
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2013-09-24 21:51:22 PDT
Tie the life of DocumentStyleSheetCollection and Document together
Comment 1 Benjamin Poulain 2013-09-24 21:54:36 PDT
Created attachment 212535 [details]
Patch
Comment 2 Andreas Kling 2013-09-25 12:28:04 PDT
Comment on attachment 212535 [details]
Patch

Great idea!
Comment 3 Benjamin Poulain 2013-09-25 14:13:09 PDT
Committed r156422: <http://trac.webkit.org/changeset/156422>
Comment 4 Roger Fong 2013-09-25 15:30:29 PDT
On the Windows debug tester the following assertion now fails:

ASSERTION FAILED: documentInternal()
c:\cygwin\home\buildbot\slave\win-debug\build\webkitbuild\debug\include\webcore\Node.h(403) : WebCore::Node::document

See http://build.webkit.org/results/Apple%20Win%207%20Debug%20(Tests)/r156423%20(54751)/results.html
Comment 5 Benjamin Poulain 2013-09-25 15:40:36 PDT
That's odd. Could you symbolicate one of the crash?
Comment 6 Jer Noble 2013-09-25 16:55:03 PDT
(In reply to comment #5)
> That's odd. Could you symbolicate one of the crash?

I'm hitting this very consistently.

/Volumes/Users/jer/Projects/WebKit.git/OpenSource/Source/WebCore/dom/Node.h(403) : WebCore::Document &WebCore::Node::document() const
1   0x102e511b0 WTFCrash
2   0x10592b9c3 WebCore::Node::document() const
3   0x105cad2b1 WebCore::CSSStyleSheet::ownerDocument() const
4   0x105cad2f9 WebCore::CSSStyleSheet::clearOwnerNode()
5   0x105e1cb1b WebCore::DocumentStyleSheetCollection::~DocumentStyleSheetCollection()
6   0x105e1cac5 WebCore::DocumentStyleSheetCollection::~DocumentStyleSheetCollection()
7   0x105db1ddc WebCore::Document::~Document()
8   0x1061cea45 WebCore::HTMLDocument::~HTMLDocument()
9   0x1061ce915 WebCore::HTMLDocument::~HTMLDocument()
10  0x1061ce8e9 WebCore::HTMLDocument::~HTMLDocument()
11  0x1061ce93c non-virtual thunk to WebCore::HTMLDocument::~HTMLDocument()

It looks like we're now referencing a node's document from the document's destructor.  Seems bad.
Comment 7 Jer Noble 2013-09-25 16:58:26 PDT
Aha, now that it's a non-pointer member, we're destroying it after calling clearDocumentScope() rather than before.  That causes the ASSERT we're seeing in Node::document().
Comment 8 Benjamin Poulain 2013-09-25 17:38:59 PDT
(In reply to comment #7)
> Aha, now that it's a non-pointer member, we're destroying it after calling clearDocumentScope() rather than before.  That causes the ASSERT we're seeing in Node::document().

I think CSSStyleSheet should not re-enter document at any point during destruction.

I am trying something to that effect. It is building at the moment.
Comment 9 Benjamin Poulain 2013-09-25 18:18:38 PDT
Follow up: https://bugs.webkit.org/show_bug.cgi?id=121933