Bug 103242 – Circular reference between Document and MediaQueryMatcher.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marja Hölttä]

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.

[Marja Hölttä]

Created attachment 175973 [details]
Patch

[WebKit Review Bot]

Comment on attachment 175973 [details]
Patch

Clearing flags on attachment: 175973

Committed r135709: <http://trac.webkit.org/changeset/135709>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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.

[Adam Barth]

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?

[Marja Hölttä]

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.

[Yongjun Zhang]

(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()?