Prevent more script-observable cases of HTMLMediaElement GC

Source/core/html/HTMLMediaElement.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 364 matching lines @@
 
     setHasCustomStyleCallbacks();
     addElementToDocumentMap(this, &document);
 }
 
 HTMLMediaElement::~HTMLMediaElement()
 {
     WTF_LOG(Media, "HTMLMediaElement::~HTMLMediaElement(%p)", this);
 
 #if ENABLE(OILPAN)
-    // If the HTMLMediaElement dies with the document we are not

[Mads Ager (chromium), 2014/09/15 07:01:18]

From an Oilpan perspective it is really nice to get rid of this!

One verifying question: loading on an HTMLMediaElement can only be started from
JavaScript? So we know that there is a JavaScript wrapper for the
HTMLMediaElement when m_shouldDelayLoadEvent is set to true? If that is the
case, this looks correct to me. If there is a wrapper when loading starts the
hasPendingActivity check will keep the wrapper (and therefore the
HTMLMediaElement) alive until the load is completed.

[philipj_slow, 2014/09/15 13:07:44]

No, loading can start without a wrapper, all you need for that to happen is
<video src="video.webm"> and no JavaScript. (m_shouldDelayLoadEvent corresponds
to the spec's delaying-the-load-event flag, which can be triggered by the HTML
parser inserting elements.)

-    // allowed to touch the document to adjust delay load event counts
-    // because the document could have been already
-    // destructed. However, if the HTMLMediaElement dies with the
-    // document there is no need to change the delayed load counts
-    // because no load event will fire anyway. If the document is
-    // still alive we do have to decrement the load delay counts. We
-    // determine if the document is alive via the ActiveDOMObject
-    // which is a context lifecycle observer. If the Document has been
-    // destructed ActiveDOMObject::executionContext() returns 0.
-    if (ActiveDOMObject::executionContext())
-        setShouldDelayLoadEvent(false);
+    ASSERT(!m_shouldDelayLoadEvent);
 #else
     // HTMLMediaElement and m_asyncEventQueue always become unreachable
     // together. So HTMLMediaElemenet and m_asyncEventQueue are destructed in
     // the same GC. We don't need to close it explicitly in Oilpan.
     m_asyncEventQueue->close();
 
     setShouldDelayLoadEvent(false);

[Mads Ager (chromium), 2014/09/15 07:01:18]

This should be ASSERT(!m_shouldDelayLoadEvent) as well, right?

The same reasoning should apply to the non-oilpan code.

[philipj_slow, 2014/09/15 13:07:44]

That was the previous CL, and the assert failed as described in
https://codereview.chromium.org/552303006/#msg23

 
     if (m_textTracks)
         m_textTracks->clearOwner();
     m_audioTracks->shutdown();
     m_videoTracks->shutdown();
 
     if (m_mediaController) {
         m_mediaController->removeMediaElement(this);
         m_mediaController = nullptr;
     }
@@ skipping 3047 matching lines @@
         renderer()->updateFromElement();
 
     stopPeriodicTimers();
     cancelPendingEventsAndCallbacks();
 
     m_asyncEventQueue->close();
 }
 
 bool HTMLMediaElement::hasPendingActivity() const
 {
-    return (hasAudio() && isPlaying()) || m_asyncEventQueue->hasPendingEvents();
+    if (!document().isActive())
+        return false;
+
+    if (m_shouldDelayLoadEvent)
+        return true;
+
+    if (m_networkState == NETWORK_LOADING)
+        return true;
+
+    if (potentiallyPlaying())
+        return true;
+
+    if (m_seeking)
+        return true;
+
+    if (m_asyncEventQueue->hasPendingEvents())
+        return true;
+
+    if (m_mediaSource)
+        return true;
+
+    return false;
 }
 
 void HTMLMediaElement::contextDestroyed()
 {
     // With Oilpan the ExecutionContext is weakly referenced from the media
     // controller and so it will clear itself on destruction.
 #if !ENABLE(OILPAN)
     if (m_mediaController)
         m_mediaController->clearExecutionContext();
 #endif
@@ skipping 501 matching lines @@
 
 #if ENABLE(WEB_AUDIO)
 void HTMLMediaElement::clearWeakMembers(Visitor* visitor)
 {
     if (!visitor->isAlive(m_audioSourceNode) && audioSourceProvider())
         audioSourceProvider()->setClient(0);
 }
 #endif
 
 }