Bug 1325878: Only deep-clone StyleSheet::mMedia when the document changes. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 10 Apr 2017 22:44:10 +0800
changeset 560405 c9d23b739bbac82fc17db60756eb6598e20f8838
parent 560404 4ab509fd58b57adead0088a13aa8705746ee06c8
child 560406 84591da95855900495c2f49aadebd9476277d4f7
push id53394
push userbmo:emilio+bugs@crisal.io
push dateTue, 11 Apr 2017 09:32:35 +0000
reviewersheycam
bugs1325878
milestone55.0a1
Bug 1325878: Only deep-clone StyleSheet::mMedia when the document changes. r?heycam This quite conveniently fixes a Servo crash when double-borrowing the style lock while inserting an @import rule. I could tweak the algorithm to avoid the double borrow, but I can also fix this I guess. MozReview-Commit-ID: KE8fQghzuuH
layout/style/StyleSheet.cpp
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/cssom/import-rule-stylesheet-media.html
--- a/layout/style/StyleSheet.cpp
+++ b/layout/style/StyleSheet.cpp
@@ -43,22 +43,16 @@ StyleSheet::StyleSheet(const StyleSheet&
   , mDisabled(aCopy.mDisabled)
     // We only use this constructor during cloning.  It's the cloner's
     // responsibility to notify us if we end up being owned by a document.
   , mDocumentAssociationMode(NotOwnedByDocument)
   , mInner(aCopy.mInner) // Shallow copy, but concrete subclasses will fix up.
 {
   MOZ_ASSERT(mInner, "Should only copy StyleSheets with an mInner.");
   mInner->AddSheet(this);
-
-  if (aCopy.mMedia) {
-    // XXX This is wrong; we should be keeping @import rules and
-    // sheets in sync!
-    mMedia = aCopy.mMedia->Clone();
-  }
 }
 
 StyleSheet::~StyleSheet()
 {
   MOZ_ASSERT(mInner, "Should have an mInner at time of destruction.");
   MOZ_ASSERT(mInner->mSheets.Contains(this), "Our mInner should include us.");
   mInner->RemoveSheet(this);
   mInner = nullptr;
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -84407,16 +84407,22 @@
     ]
    ],
    "cssom/historical.html": [
     [
      "/cssom/historical.html",
      {}
     ]
    ],
+   "cssom/import-rule-stylesheet-media.html": [
+    [
+     "/cssom/import-rule-stylesheet-media.html",
+     {}
+    ]
+   ],
    "cssom/overflow-serialization.html": [
     [
      "/cssom/overflow-serialization.html",
      {}
     ]
    ],
    "cssom/serialization-CSSDeclaration-with-important.html": [
     [
@@ -161368,16 +161374,20 @@
   "cssom/StyleSheetList.html": [
    "3a0e6f64f70f863d679e537c4bfb76aaa0d3598a",
    "testharness"
   ],
   "cssom/historical.html": [
    "2c78218b89efb9bdf60cf708920be142051347c7",
    "testharness"
   ],
+  "cssom/import-rule-stylesheet-media.html": [
+   "79b4a278f0e35646cfdffeebf8f0523e2772bc9b",
+   "testharness"
+  ],
   "cssom/overflow-serialization.html": [
    "199039706289f577652b968706fc1251398acd1c",
    "testharness"
   ],
   "cssom/serialization-CSSDeclaration-with-important.html": [
    "ecc8b95fb2d71cacee271f4fea2fc16f35cdba57",
    "testharness"
   ],
@@ -179653,17 +179663,17 @@
    "997cee37dcd202498196e63e0f66035979121b7f",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/nomodule-reflect.html": [
    "ac2b3c16e9e9263cd4c14de205b63709c14ec2e3",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/nomodule-set-on-async-classic-script.html": [
-   "5b4a532b21caa6235bed10a28878c65523a816aa",
+   "6ef870db74ba0dbb6171c74437a78cef4a6f6062",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/nomodule-set-on-external-module-script.html": [
    "f43755d9dffe2983a377f2c00b855f106776b617",
    "testharness"
   ],
   "html/semantics/scripting-1/the-script-element/nomodule-set-on-inline-classic-scripts.html": [
    "2b6654342c5a2e2d85df2bc8ec805b5fa7ed1550",
@@ -202252,20 +202262,16 @@
   "service-workers/service-worker/registration-iframe.https.html": [
    "6d11b9ecf339e6e476fe594d5cb4e0873b0845d1",
    "testharness"
   ],
   "service-workers/service-worker/registration-service-worker-attributes.https.html": [
    "7c49ce1c6170733033add6253a3f4a7e0483452e",
    "testharness"
   ],
-  "service-workers/service-worker/registration-useCache.https.html": [
-   "73d662eafa93ca3dee4a4e5d34623cf069c2b8f8",
-   "testharness"
-  ],
   "service-workers/service-worker/registration.https.html": [
    "0a06c368a14c008c385c9df3cde35f090d96d58b",
    "testharness"
   ],
   "service-workers/service-worker/rejections.https.html": [
    "785a18ac3c8001034f583a8e97195aa47093bd0d",
    "testharness"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/cssom/import-rule-stylesheet-media.html
@@ -0,0 +1,15 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>CSS Test: Script-generated @import rules keep their media in sync with their associated stylesheet.</title>
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<style>
+</style>
+<script>
+test(function() {
+  document.styleSheets[0].insertRule("@import \"t.css\";", 0);
+  var importRule = document.styleSheets[0].cssRules[0];
+  importRule.media.mediaText = "screen";
+  assert_equals(importRule.media.mediaText, importRule.styleSheet.media.mediaText);
+}, "@import rules and the associated CSS media stay in Sync.");
+</script>