Bug 610956. Don't set up loadListener as a shutdown observer or set our mChannel to the new channel until we're sure we're doing a load. r=roc
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 16 Nov 2010 08:26:48 -0500
changeset 57574 f9e32506519f880bf8f548cd285f813da12d1d8c
parent 57573 6b25735eebd08801845e958e0f88ee5c1d57f79a
child 57575 410c9f17f0fc4edba06909012cbe4db38d0d8eea
push id16988
push userbzbarsky@mozilla.com
push dateTue, 16 Nov 2010 13:27:25 +0000
treeherdermozilla-central@410c9f17f0fc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs610956
milestone2.0b8pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 610956. Don't set up loadListener as a shutdown observer or set our mChannel to the new channel until we're sure we're doing a load. r=roc
content/html/content/src/nsHTMLMediaElement.cpp
--- a/content/html/content/src/nsHTMLMediaElement.cpp
+++ b/content/html/content/src/nsHTMLMediaElement.cpp
@@ -938,86 +938,76 @@ nsresult nsHTMLMediaElement::LoadResourc
   nsCOMPtr<nsIContentSecurityPolicy> csp;
   rv = NodePrincipal()->GetCsp(getter_AddRefs(csp));
   NS_ENSURE_SUCCESS(rv,rv);
   if (csp) {
     channelPolicy = do_CreateInstance("@mozilla.org/nschannelpolicy;1");
     channelPolicy->SetContentSecurityPolicy(csp);
     channelPolicy->SetLoadType(nsIContentPolicy::TYPE_MEDIA);
   }
-  rv = NS_NewChannel(getter_AddRefs(mChannel),
+  nsCOMPtr<nsIChannel> channel;
+  rv = NS_NewChannel(getter_AddRefs(channel),
                      aURI,
                      nsnull,
                      loadGroup,
                      nsnull,
                      nsICachingChannel::LOAD_BYPASS_LOCAL_CACHE_IF_BUSY,
                      channelPolicy);
   NS_ENSURE_SUCCESS(rv,rv);
 
-  // The listener holds a strong reference to us.  This creates a reference
-  // cycle which is manually broken in the listener's OnStartRequest method
-  // after it is finished with the element. The cycle will also be
-  // broken if we get a shutdown notification before OnStartRequest fires.
-  // Necko guarantees that OnStartRequest will eventually fire if we
-  // don't shut down first.
+  // The listener holds a strong reference to us.  This creates a
+  // reference cycle, once we've set mChannel, which is manually broken
+  // in the listener's OnStartRequest method after it is finished with
+  // the element. The cycle will also be broken if we get a shutdown
+  // notification before OnStartRequest fires.  Necko guarantees that
+  // OnStartRequest will eventually fire if we don't shut down first.
   nsRefPtr<MediaLoadListener> loadListener = new MediaLoadListener(this);
-  if (!loadListener) return NS_ERROR_OUT_OF_MEMORY;
 
-  // loadListener will be unregistered either on shutdown or when
-  // OnStartRequest fires.
-  nsContentUtils::RegisterShutdownObserver(loadListener);
-  mChannel->SetNotificationCallbacks(loadListener);
+  channel->SetNotificationCallbacks(loadListener);
 
   nsCOMPtr<nsIStreamListener> listener;
   if (ShouldCheckAllowOrigin()) {
-    nsCrossSiteListenerProxy* crossSiteListener =
+    listener =
       new nsCrossSiteListenerProxy(loadListener,
                                    NodePrincipal(),
-                                   mChannel,
+                                   channel,
                                    PR_FALSE,
                                    &rv);
-    listener = crossSiteListener;
-    NS_ENSURE_TRUE(crossSiteListener, NS_ERROR_OUT_OF_MEMORY);
-    NS_ENSURE_SUCCESS(rv, rv);
   } else {
     rv = nsContentUtils::GetSecurityManager()->
            CheckLoadURIWithPrincipal(NodePrincipal(),
                                      aURI,
                                      nsIScriptSecurityManager::STANDARD);
-    NS_ENSURE_SUCCESS(rv,rv);
     listener = loadListener;
   }
+  NS_ENSURE_SUCCESS(rv, rv);
 
-  nsCOMPtr<nsIHttpChannel> hc = do_QueryInterface(mChannel);
+  nsCOMPtr<nsIHttpChannel> hc = do_QueryInterface(channel);
   if (hc) {
     // Use a byte range request from the start of the resource.
     // This enables us to detect if the stream supports byte range
     // requests, and therefore seeking, early.
     hc->SetRequestHeader(NS_LITERAL_CSTRING("Range"),
                          NS_LITERAL_CSTRING("bytes=0-"),
                          PR_FALSE);
 
     SetRequestHeaders(hc);
   }
 
-  rv = mChannel->AsyncOpen(listener, nsnull);
-  if (NS_FAILED(rv)) {
-    // OnStartRequest is guaranteed to be called if the open succeeds.  If
-    // the open failed, the listener's OnStartRequest will never be called,
-    // so we need to break the element->channel->listener->element reference
-    // cycle here.  The channel holds the only reference to the listener,
-    // and is useless now anyway, so drop our reference to it to allow it to
-    // be destroyed.
-    mChannel = nsnull;
-    return rv;
-  }
+  rv = channel->AsyncOpen(listener, nsnull);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   // Else the channel must be open and starting to download. If it encounters
   // a non-catastrophic failure, it will set a new task to continue loading
-  // another candidate.
+  // another candidate.  It's safe to set it as mChannel now.
+  mChannel = channel;
+
+  // loadListener will be unregistered either on shutdown or when
+  // OnStartRequest for the channel we just opened fires.
+  nsContentUtils::RegisterShutdownObserver(loadListener);
   return NS_OK;
 }
 
 nsresult nsHTMLMediaElement::LoadWithChannel(nsIChannel *aChannel,
                                              nsIStreamListener **aListener)
 {
   NS_ENSURE_ARG_POINTER(aChannel);
   NS_ENSURE_ARG_POINTER(aListener);