Bug 103242

Summary: Circular reference between Document and MediaQueryMatcher.
Product: WebKit Reporter: Marja Hölttä <marja>
Component: New BugsAssignee: Marja Hölttä <marja>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, beidson, hayato, jochen, kenneth, luiz, marcoos+bwo, ojan, webkit.review.bot, yongjun_zhang
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Marja Hölttä 2012-11-26 04:38:14 PST
Document has a RefPtr<MediaQueryMatcher>, and in ~Document it does this:

if (m_mediaQueryMatcher)
    m_mediaQueryMatcher->documentDestroyed();

However, MediaQueryMatcher also references Document:

MediaQueryMatcher has Vector<OwnPtr<Listener> > m_listeners:
- MediaQueryMatcher::Listener has RefPtr<MediaQueryListListener> m_listener;
- MediaQueryListListener has ScriptValue m_value;
- In V8, ScriptValue has ScopedPersistent<v8::Value> m_value; and it ends up keeping the Document alive.

This reference path is cleared in MediaQueryMatcher::documentDestroyed() which destroys the MediaQueryListListeners.

So, for example, if a tab is reloaded in Chrome, and there are MediaQueryListListeners, ~Document never happens. This causes www.crbug.com/113983.
Comment 1 Marja Hölttä 2012-11-26 05:00:19 PST
Created attachment 175973 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-26 05:42:06 PST
Comment on attachment 175973 [details]
Patch

Clearing flags on attachment: 175973

Committed r135709: <http://trac.webkit.org/changeset/135709>
Comment 3 WebKit Review Bot 2012-11-26 05:42:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Alexey Proskuryakov 2012-11-26 09:32:42 PST
Is it correct that a document that has been detached can never be re-attached (and we'll never have such a code path)?

>     if (m_mediaQueryMatcher) 
>        m_mediaQueryMatcher->documentDestroyed(); 

Calling this function from a function other than destructor is not telling the truth. You should have renamed it to documentDetached.
Comment 5 Adam Barth 2012-11-26 10:48:23 PST
Comment on attachment 175973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175973&action=review

> Source/WebCore/dom/Document.cpp:2144
> +    if (m_mediaQueryMatcher)
> +        m_mediaQueryMatcher->documentDestroyed();

Why does the m_mediaQueryMatcher need special handling here?  Can't it use one of our more general mechanisms for getting notified about the Document's lifecycle?
Comment 6 Marja Hölttä 2012-11-27 02:22:39 PST
Update:

We're discussing with abarth@ what is the right way to fix this bug. So I won't do the name change documentDestroyed -> documentDetached before the real fix.

This quick and dirty fix was merged to Chromium, and I'm willing to work on the real fix, as soon as we figure out how it should be done.
Comment 7 Yongjun Zhang 2012-12-11 13:54:20 PST
(In reply to comment #6)
> Update:
> 
> We're discussing with abarth@ what is the right way to fix this bug. So I won't do the name change documentDestroyed -> documentDetached before the real fix.
> 
> This quick and dirty fix was merged to Chromium, and I'm willing to work on the real fix, as soon as we figure out how it should be done.

Given the fact that this circular reference is caused by matchMedia listeners, I guess we could add a method removeAllListeners to MediaQueryMatcher and call that in Document::removeAllEventListeners()?