Bug 1533783 - Avoid crashing when calling insertRule("@import ...") on a detached sheet. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 14 Mar 2019 22:30:37 +0000
changeset 521983 5ce27c44f79e
parent 521982 c0613d0cb732
child 521984 5621ae8031ce
child 522012 c525a24dffc3
push id10870
push usernbeleuzu@mozilla.com
push dateFri, 15 Mar 2019 20:00:07 +0000
treeherdermozilla-beta@c594aee5b7a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1533783
milestone67.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 1533783 - Avoid crashing when calling insertRule("@import ...") on a detached sheet. r=heycam This should unblock the fuzzers for now, though it's not the ideal solution. It's the only reasonably easy solution to unblock them though, I think. We should probably always keep track of the document a stylesheet was associated with. We'll need that for constructible stylesheets anyway. That requires some though on how to get the cycle-collection and such right, though, and I wouldn't be able to write or land that ASAP. Differential Revision: https://phabricator.services.mozilla.com/D23584
layout/style/ServoCSSRuleList.cpp
layout/style/crashtests/1533783.html
layout/style/crashtests/crashtests.list
servo/components/style/stylesheets/rule_parser.rs
--- a/layout/style/ServoCSSRuleList.cpp
+++ b/layout/style/ServoCSSRuleList.cpp
@@ -152,16 +152,22 @@ void ServoCSSRuleList::DropParentRuleRef
 
 nsresult ServoCSSRuleList::InsertRule(const nsAString& aRule, uint32_t aIndex) {
   MOZ_ASSERT(mStyleSheet,
              "Caller must ensure that "
              "the list is not unlinked from stylesheet");
   NS_ConvertUTF16toUTF8 rule(aRule);
   bool nested = !!mParentRule;
   css::Loader* loader = nullptr;
+
+  // TODO(emilio, bug 1535456): Should probably always be able to get a handle
+  // to some loader if we're parsing an @import rule, but which one?
+  //
+  // StyleSheet::ReparseSheet just mints a new loader, but that'd be wrong in
+  // this case I think, since such a load will bypass CSP checks.
   if (Document* doc = mStyleSheet->GetAssociatedDocument()) {
     loader = doc->CSSLoader();
   }
   uint16_t type;
   nsresult rv =
       Servo_CssRules_InsertRule(mRawRules, mStyleSheet->RawContents(), &rule,
                                 aIndex, nested, loader, mStyleSheet, &type);
   if (NS_FAILED(rv)) {
new file mode 100644
--- /dev/null
+++ b/layout/style/crashtests/1533783.html
@@ -0,0 +1,10 @@
+<script>
+  function start() {
+    const style = document.createElement('style')
+    document.head.appendChild(style)
+    const sheet_1 = style.sheet
+    document.head.appendChild(style)
+    sheet_1.insertRule('@import url()', 0)
+  }
+  document.addEventListener('DOMContentLoaded', start)
+</script>
--- a/layout/style/crashtests/crashtests.list
+++ b/layout/style/crashtests/crashtests.list
@@ -294,8 +294,9 @@ load 1475003.html
 load 1479681.html
 load 1488817.html
 load 1490012.html
 load 1502893.html
 load 1509989.html
 load 1514086.html
 pref(layout.css.moz-binding.content.enabled,false) load 1517319.html
 load 1533891.html
+load 1533783.html
--- a/servo/components/style/stylesheets/rule_parser.rs
+++ b/servo/components/style/stylesheets/rule_parser.rs
@@ -184,16 +184,23 @@ impl<'a, 'i> AtRuleParser<'i> for TopLev
         input: &mut Parser<'i, 't>,
     ) -> Result<AtRuleType<AtRuleNonBlockPrelude, AtRuleBlockPrelude>, ParseError<'i>> {
         match_ignore_ascii_case! { &*name,
             "import" => {
                 if !self.check_state(State::Imports) {
                     return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedImportRule))
                 }
 
+                // FIXME(emilio): We should always be able to have a loader
+                // around! See bug 1533783.
+                if self.loader.is_none() {
+                    error!("Saw @import rule, but no way to trigger the load");
+                    return Err(input.new_custom_error(StyleParseErrorKind::UnexpectedImportRule))
+                }
+
                 let url_string = input.expect_url_or_string()?.as_ref().to_owned();
                 let url = CssUrl::parse_from_string(url_string, &self.context);
 
                 let media = MediaList::parse(&self.context, input);
                 let media = Arc::new(self.shared_lock.wrap(media));
 
                 let prelude = AtRuleNonBlockPrelude::Import(url, media);
                 return Ok(AtRuleType::WithoutBlock(prelude));