Bug 816498: Fix some things about background attribute handling that are still broken. r=bz
authorKyle Huey <khuey@kylehuey.com>
Sat, 23 Feb 2013 06:59:43 -0800
changeset 122760 8ee9a76cb582fa8279c18cce6aba05d2326c414c
parent 122759 cd6d61e427628dc0528be509cefd0a4f5e49d6c1
child 122761 5c3d85cb3cefb9fc650e79977ca248bc752ebaaf
push id23472
push userkhuey@mozilla.com
push dateSat, 23 Feb 2013 15:11:48 +0000
treeherdermozilla-inbound@8ee9a76cb582 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs816498, 560235
milestone22.0a1
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 816498: Fix some things about background attribute handling that are still broken. r=bz As filed the bug is about table backgrounds failing to print. The root cause of this is that we load images from the original document, because print/print-preview documents (which are static clones of the original) cannot do loads. This results in an entry in the css::ImageValue's request table under the original document, but entry under the printing document. Then we do layout, try to get the request, and fail, and nothing is displayed. The solution to this is to force us to clone the request for the printing document if we're loading off the original document. I manually verified that this does not regress Bug 560235. While writing a test for this, we discovered another problem. The reftest print code does not actually use the printing codepath. Instead it takes an existing document, tears down its presshell, and creates a new presshell for printing. Fixing the above bug did not make the reftest print test pass because ImageLoader does not deal properly with presshell destruction/recreation. It assumes that when the presshell is destroyed all of the css::ImageValues can be cleared of their entries for that document. This fails for ImageValues for mapped attributes because they are held alive by the content tree. When a new presshell for this document is then created there is no entry for the document and thus no image request to paint. The fix for this is to only clear the frame to request and request to frame mapping hashtables and not to clear the document's entry on the ImageValue when the presshell is destroyed. The destruction of the ImageLoader (which is held from a strong reference on the document) is when those entries are removed. The final change is to change css::ImageValue's hashtable from holding a strong reference to the keys (which are documents) to holding raw pointer references. This is safe because we clear the relevant entry when the corresponding ImageLoader dies, and is needed to prevent a reference cycle that was being broken by presshell destruction.
layout/base/nsPresShell.cpp
layout/reftests/backgrounds/reftest.list
layout/reftests/backgrounds/table-background-print-ref.html
layout/reftests/backgrounds/table-background-print.html
layout/reftests/backgrounds/table-background-ref.html
layout/reftests/backgrounds/table-background.html
layout/reftests/bugs/816948-1-ref.html
layout/reftests/bugs/816948-1.html
layout/reftests/bugs/816948-iframe.html
layout/reftests/bugs/reftest.list
layout/style/ImageLoader.cpp
layout/style/ImageLoader.h
layout/style/nsCSSValue.cpp
layout/style/nsCSSValue.h
--- a/layout/base/nsPresShell.cpp
+++ b/layout/base/nsPresShell.cpp
@@ -1987,17 +1987,17 @@ PresShell::FireResizeEvent()
 }
 
 void
 PresShell::SetIgnoreFrameDestruction(bool aIgnore)
 {
   if (mDocument) {
     // We need to tell the ImageLoader to drop all its references to frames
     // because they're about to go away and it won't get notifications of that.
-    mDocument->StyleImageLoader()->ClearAll();
+    mDocument->StyleImageLoader()->ClearFrames();
   }
   mIgnoreFrameDestruction = aIgnore;
 }
 
 void
 PresShell::NotifyDestroyingFrame(nsIFrame* aFrame)
 {
   if (!mIgnoreFrameDestruction) {
--- a/layout/reftests/backgrounds/reftest.list
+++ b/layout/reftests/backgrounds/reftest.list
@@ -128,15 +128,16 @@ fails == background-size-zoom-repeat.htm
 random-if(bug685516||B2G) == fixed-bg-with-transform-outside-viewport-1.html fixed-bg-with-transform-outside-viewport-ref.html
 
 random-if(bug685516) HTTP == root-background-1.html root-background-ref.html
 random-if(bug685516) HTTP != root-background-1.html about:blank
 
 random-if(bug685516||B2G) == really-big-background.html really-big-background-ref.html
 random-if(bug685516) == body-background.html body-background-ref.html
 random-if(bug685516) == table-background.html table-background-ref.html
+random-if(bug685516) == table-background-print.html table-background-print-ref.html
 random-if(bug685516) != div-background.html div-background-ref.html
 
 random-if(bug685516) == background-repeat-1-ref.html background-repeat-1.html
 
 random-if(bug685516) == multi-background-clip-content-border.html multi-background-clip-content-border-ref.html
 
 HTTP == background-referrer.html background-referrer-ref.html
copy from layout/reftests/backgrounds/table-background-ref.html
copy to layout/reftests/backgrounds/table-background-print-ref.html
--- a/layout/reftests/backgrounds/table-background-ref.html
+++ b/layout/reftests/backgrounds/table-background-print-ref.html
@@ -1,8 +1,9 @@
+<html class="reftest-print">
 <body>
 <table style="background-image: url('aqua-yellow-32x32.png')">
   <thead style="background-image: url('blue-16x20-green-16x20.png')">
     <tr>
       <td>
         Foo
       </td>
       <td style="background-image: url('yellow-32x32.png')">
@@ -32,8 +33,9 @@
     <tr>
       <td>
         Baz
       </td>
     </tr>
   </tfoot>
 </table>
 </body>
+</html>
copy from layout/reftests/backgrounds/table-background.html
copy to layout/reftests/backgrounds/table-background-print.html
--- a/layout/reftests/backgrounds/table-background.html
+++ b/layout/reftests/backgrounds/table-background-print.html
@@ -1,8 +1,9 @@
+<html class="reftest-print">
 <body>
 <table background="aqua-yellow-32x32.png">
   <thead background="blue-16x20-green-16x20.png">
     <tr>
       <td>
         Foo
       </td>
       <td background="yellow-32x32.png">
@@ -32,8 +33,9 @@
     <tr>
       <td>
         Baz
       </td>
     </tr>
   </tfoot>
 </table>
 </body>
+</html>
--- a/layout/reftests/backgrounds/table-background-ref.html
+++ b/layout/reftests/backgrounds/table-background-ref.html
@@ -1,8 +1,9 @@
+<html>
 <body>
 <table style="background-image: url('aqua-yellow-32x32.png')">
   <thead style="background-image: url('blue-16x20-green-16x20.png')">
     <tr>
       <td>
         Foo
       </td>
       <td style="background-image: url('yellow-32x32.png')">
@@ -32,8 +33,9 @@
     <tr>
       <td>
         Baz
       </td>
     </tr>
   </tfoot>
 </table>
 </body>
+</html>
--- a/layout/reftests/backgrounds/table-background.html
+++ b/layout/reftests/backgrounds/table-background.html
@@ -1,8 +1,9 @@
+<html>
 <body>
 <table background="aqua-yellow-32x32.png">
   <thead background="blue-16x20-green-16x20.png">
     <tr>
       <td>
         Foo
       </td>
       <td background="yellow-32x32.png">
@@ -32,8 +33,9 @@
     <tr>
       <td>
         Baz
       </td>
     </tr>
   </tfoot>
 </table>
 </body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/816948-1-ref.html
@@ -0,0 +1,6 @@
+<html>
+<body>
+<iframe id="ifr" src="816948-iframe.html">
+</iframe>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/816948-1.html
@@ -0,0 +1,16 @@
+<html class="reftest-wait">
+<script>
+function boom() {
+  var ifr = document.getElementById("ifr");
+  ifr.style.display = "none";
+  // flush layout
+  document.documentElement.offsetLeft;
+  ifr.style.display = "block";
+  document.documentElement.removeAttribute("class");
+}
+</script>
+<body onload="boom();">
+<iframe id="ifr" src="816948-iframe.html">
+</iframe>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/816948-iframe.html
@@ -0,0 +1,3 @@
+<body background="mozilla-banner.gif">
+foopy
+</body>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1734,15 +1734,16 @@ fuzzy(40,800) == 797797-2.html 797797-2-
 == 804323-1.html 804323-1-ref.html
 == 811301-1.html 811301-1-ref.html
 == 812824-1.html 812824-1-ref.html
 == 814677.html 814677-ref.html
 skip-if(B2G) == 815593-1.html 815593-1-ref.html
 skip-if(B2G) == 814952-1.html 814952-1-ref.html
 == 816458-1.html 816458-1-ref.html
 == 816359-1.html 816359-1-ref.html
+== 816948-1.html 816948-1-ref.html
 == 817019-1.html about:blank
 skip-if(B2G) == 818276-1.html 818276-1-ref.html
 == 825999.html 825999-ref.html
 == 827577-1a.html 827577-1-ref.html
 == 827577-1b.html 827577-1-ref.html
 == 827799-1.html about:blank
 == 836844-1.html 836844-1-ref.html
--- a/layout/style/ImageLoader.cpp
+++ b/layout/style/ImageLoader.cpp
@@ -38,20 +38,37 @@ ImageLoader::SetAnimationModeEnumerator(
   }
 
   // This can fail if the image is in error, and we don't care.
   container->SetAnimationMode(*mode);
 
   return PL_DHASH_NEXT;
 }
 
+static PLDHashOperator
+ClearImageHashSet(nsPtrHashKey<ImageLoader::Image>* aKey, void* aClosure)
+{
+  nsIDocument* doc = static_cast<nsIDocument*>(aClosure);
+  ImageLoader::Image* image = aKey->GetKey();
+
+  imgIRequest* request = image->mRequests.GetWeak(doc);
+  if (request) {
+    request->CancelAndForgetObserver(NS_BINDING_ABORTED);
+  }
+
+  image->mRequests.Remove(doc);
+
+  return PL_DHASH_REMOVE;
+}
+
 void
 ImageLoader::DropDocumentReference()
 {
-  ClearAll();
+  ClearFrames();
+  mImages.EnumerateEntries(&ClearImageHashSet, mDocument);
   mDocument = nullptr;
 }
 
 void
 ImageLoader::AssociateRequestToFrame(imgIRequest* aRequest,
                                      nsIFrame* aFrame)
 {
   MOZ_ASSERT(mRequestToFrameMap.IsInitialized() &&
@@ -216,38 +233,21 @@ ImageLoader::SetAnimationMode(uint16_t a
   NS_ASSERTION(aMode == imgIContainer::kNormalAnimMode ||
                aMode == imgIContainer::kDontAnimMode ||
                aMode == imgIContainer::kLoopOnceAnimMode,
                "Wrong Animation Mode is being set!");
 
   mRequestToFrameMap.EnumerateRead(SetAnimationModeEnumerator, &aMode);
 }
 
-static PLDHashOperator
-ClearImageHashSet(nsPtrHashKey<ImageLoader::Image>* aKey, void* aClosure)
-{
-  nsIDocument* doc = static_cast<nsIDocument*>(aClosure);
-  ImageLoader::Image* image = aKey->GetKey();
-
-  imgIRequest* request = image->mRequests.GetWeak(doc);
-  if (request) {
-    request->CancelAndForgetObserver(NS_BINDING_ABORTED);
-  }
-
-  image->mRequests.Remove(doc);
-
-  return PL_DHASH_REMOVE;
-}
-
 void
-ImageLoader::ClearAll()
+ImageLoader::ClearFrames()
 {
   mRequestToFrameMap.Clear();
   mFrameToRequestMap.Clear();
-  mImages.EnumerateEntries(&ClearImageHashSet, mDocument);
 }
 
 void
 ImageLoader::LoadImage(nsIURI* aURI, nsIPrincipal* aOriginPrincipal,
                        nsIURI* aReferrer, ImageLoader::Image* aImage)
 {
   NS_ASSERTION(aImage->mRequests.Count() == 0, "Huh?");
 
--- a/layout/style/ImageLoader.h
+++ b/layout/style/ImageLoader.h
@@ -54,17 +54,17 @@ public:
 
   void DisassociateRequestFromFrame(imgIRequest* aRequest,
                                     nsIFrame* aFrame);
 
   void DropRequestsForFrame(nsIFrame* aFrame);
 
   void SetAnimationMode(uint16_t aMode);
 
-  void ClearAll();
+  void ClearFrames();
 
   void LoadImage(nsIURI* aURI, nsIPrincipal* aPrincipal, nsIURI* aReferrer,
                  Image* aCSSValue);
 
   void DestroyRequest(imgIRequest* aRequest);
 
 private:
   // We need to be able to look up the frames associated with a request (for
--- a/layout/style/nsCSSValue.cpp
+++ b/layout/style/nsCSSValue.cpp
@@ -1716,24 +1716,32 @@ css::URLValue::SizeOfIncludingThis(nsMal
 }
 
 
 css::ImageValue::ImageValue(nsIURI* aURI, nsStringBuffer* aString,
                             nsIURI* aReferrer, nsIPrincipal* aOriginPrincipal,
                             nsIDocument* aDocument)
   : URLValue(aURI, aString, aReferrer, aOriginPrincipal)
 {
-  if (aDocument->GetOriginalDocument()) {
-    aDocument = aDocument->GetOriginalDocument();
+  // NB: If aDocument is not the original document, we may not be able to load
+  // images from aDocument.  Instead we do the image load from the original doc
+  // and clone it to aDocument.
+  nsIDocument* loadingDoc = aDocument->GetOriginalDocument();
+  if (!loadingDoc) {
+    loadingDoc = aDocument;
   }
 
   mRequests.Init();
 
-  aDocument->StyleImageLoader()->LoadImage(aURI, aOriginPrincipal, aReferrer,
-                                           this);
+  loadingDoc->StyleImageLoader()->LoadImage(aURI, aOriginPrincipal, aReferrer,
+                                            this);
+
+  if (loadingDoc != aDocument) {
+    aDocument->StyleImageLoader()->MaybeRegisterCSSImage(this);
+  }
 }
 
 static PLDHashOperator
 ClearRequestHashtable(nsISupports* aKey, nsRefPtr<imgRequestProxy>& aValue,
                       void* aClosure)
 {
   mozilla::css::ImageValue* image =
     static_cast<mozilla::css::ImageValue*>(aClosure);
--- a/layout/style/nsCSSValue.h
+++ b/layout/style/nsCSSValue.h
@@ -120,17 +120,17 @@ struct ImageValue : public URLValue {
   // this header is included all over.
   // aString must not be null.
   ImageValue(nsIURI* aURI, nsStringBuffer* aString, nsIURI* aReferrer,
              nsIPrincipal* aOriginPrincipal, nsIDocument* aDocument);
   ~ImageValue();
 
   // Inherit operator== from URLValue
 
-  nsRefPtrHashtable<nsISupportsHashKey, imgRequestProxy> mRequests; 
+  nsRefPtrHashtable<nsPtrHashKey<nsISupports>, imgRequestProxy> mRequests; 
 
   // Override AddRef and Release to not only log ourselves correctly, but
   // also so that we delete correctly without a virtual destructor
   NS_INLINE_DECL_REFCOUNTING(ImageValue)
 };
 
 }
 }