Bug 1466656: Make ErrorReporter a smaller Rust type. r?xidorn
And use the C++ ErrorReporter only to actually output errors.
ErrorReporter was so complicated because well, it was always enabled and had to
do a bunch of caching to not be (more) slow.
But since
bug 1452143 it's disabled by default, so we can simplify this setup a
lot.
Also while at it make the error reporting pref a static pref so that we don't
mutate globals from CSS parsing unless we're actually reporting errors.
MozReview-Commit-ID: AuIyvJwt7AU
--- a/layout/style/ErrorReporter.cpp
+++ b/layout/style/ErrorReporter.cpp
@@ -3,16 +3,17 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
/* diagnostic reporting for CSS style sheet parser */
#include "mozilla/css/ErrorReporter.h"
+#include "mozilla/StaticPrefs.h"
#include "mozilla/StyleSheetInlines.h"
#include "mozilla/css/Loader.h"
#include "mozilla/Preferences.h"
#include "mozilla/Services.h"
#include "mozilla/SystemGroup.h"
#include "nsCSSScanner.h"
#include "nsIConsoleService.h"
#include "nsIDocument.h"
@@ -63,40 +64,31 @@ public:
private:
nsCOMPtr<nsIURI> mURI;
nsString mSpec;
bool mPending;
};
} // namespace
-bool ErrorReporter::sReportErrors = false;
bool ErrorReporter::sInitialized = false;
static nsIConsoleService *sConsoleService;
static nsIFactory *sScriptErrorFactory;
static nsIStringBundle *sStringBundle;
static ShortTermURISpecCache *sSpecCache;
-#define CSS_ERRORS_PREF "layout.css.report_errors"
-
void
ErrorReporter::InitGlobals()
{
MOZ_RELEASE_ASSERT(NS_IsMainThread());
MOZ_ASSERT(!sInitialized, "should not have been called");
sInitialized = true;
- if (NS_FAILED(Preferences::AddBoolVarCache(&sReportErrors,
- CSS_ERRORS_PREF,
- true))) {
- return;
- }
-
nsCOMPtr<nsIConsoleService> cs = do_GetService(NS_CONSOLESERVICE_CONTRACTID);
if (!cs) {
return;
}
nsCOMPtr<nsIFactory> sf = do_GetClassObject(NS_SCRIPTERROR_CONTRACTID);
if (!sf) {
return;
@@ -126,27 +118,42 @@ namespace css {
ErrorReporter::ReleaseGlobals()
{
NS_IF_RELEASE(sConsoleService);
NS_IF_RELEASE(sScriptErrorFactory);
NS_IF_RELEASE(sStringBundle);
NS_IF_RELEASE(sSpecCache);
}
+static uint64_t
+FindInnerWindowID(const StyleSheet* aSheet, const Loader* aLoader)
+{
+ uint64_t innerWindowID = 0;
+ if (aSheet) {
+ innerWindowID = aSheet->FindOwningWindowInnerID();
+ }
+ if (innerWindowID == 0 && aLoader) {
+ if (nsIDocument* doc = aLoader->GetDocument()) {
+ innerWindowID = doc->InnerWindowID();
+ }
+ }
+ return innerWindowID;
+}
+
ErrorReporter::ErrorReporter(const StyleSheet* aSheet,
const Loader* aLoader,
nsIURI* aURI)
: mSheet(aSheet)
, mLoader(aLoader)
, mURI(aURI)
- , mInnerWindowID(0)
, mErrorLineNumber(0)
, mPrevErrorLineNumber(0)
, mErrorColNumber(0)
{
+ MOZ_ASSERT(ShouldReportErrors(mSheet, mLoader));
}
ErrorReporter::~ErrorReporter()
{
MOZ_ASSERT(NS_IsMainThread());
// Schedule deferred cleanup for cached data. We want to strike a
// balance between performance and memory usage, so we only allow
// short-term caching.
@@ -184,70 +191,51 @@ SheetOwner(const StyleSheet& aSheet)
return owner;
}
auto* associated = aSheet.GetAssociatedDocumentOrShadowRoot();
return associated ? &associated->AsNode() : nullptr;
}
bool
-ErrorReporter::ShouldReportErrors()
+ErrorReporter::ShouldReportErrors(const StyleSheet* aSheet,
+ const Loader* aLoader)
{
MOZ_ASSERT(NS_IsMainThread());
- EnsureGlobalsInitialized();
- if (!sReportErrors) {
+ if (!StaticPrefs::layout_css_report_errors()) {
return false;
}
- if (mInnerWindowID) {
- // We already reported an error, and that has cleared mSheet and mLoader, so
- // we'd get the bogus value otherwise.
- return true;
- }
-
- if (mSheet) {
- nsINode* owner = SheetOwner(*mSheet);
+ if (aSheet) {
+ nsINode* owner = SheetOwner(*aSheet);
if (owner && ShouldReportErrors(*owner->OwnerDoc())) {
return true;
}
}
- if (mLoader && mLoader->GetDocument() &&
- ShouldReportErrors(*mLoader->GetDocument())) {
+ if (aLoader && aLoader->GetDocument() &&
+ ShouldReportErrors(*aLoader->GetDocument())) {
return true;
}
return false;
}
void
ErrorReporter::OutputError()
{
MOZ_ASSERT(NS_IsMainThread());
- MOZ_ASSERT(ShouldReportErrors());
+ MOZ_ASSERT(ShouldReportErrors(mSheet, mLoader));
if (mError.IsEmpty()) {
return;
}
- if (mInnerWindowID == 0 && (mSheet || mLoader)) {
- if (mSheet) {
- mInnerWindowID = mSheet->FindOwningWindowInnerID();
- }
- if (mInnerWindowID == 0 && mLoader) {
- nsIDocument* doc = mLoader->GetDocument();
- if (doc) {
- mInnerWindowID = doc->InnerWindowID();
- }
- }
- // don't attempt this again, even if we failed
- mSheet = nullptr;
- mLoader = nullptr;
- }
+ EnsureGlobalsInitialized();
if (mFileName.IsEmpty()) {
if (mURI) {
if (!sSpecCache) {
sSpecCache = new ShortTermURISpecCache;
NS_ADDREF(sSpecCache);
}
mFileName = sSpecCache->GetSpec(mURI);
@@ -266,17 +254,17 @@ ErrorReporter::OutputError()
// an already anonymized uri spec.
rv = errorObject->InitWithSanitizedSource(mError,
mFileName,
mErrorLine,
mErrorLineNumber,
mErrorColNumber,
nsIScriptError::warningFlag,
"CSS Parser",
- mInnerWindowID);
+ FindInnerWindowID(mSheet, mLoader));
if (NS_SUCCEEDED(rv)) {
sConsoleService->LogMessage(errorObject);
}
}
ClearError();
}
@@ -314,41 +302,41 @@ void
ErrorReporter::ClearError()
{
mError.Truncate();
}
void
ErrorReporter::AddToError(const nsString &aErrorText)
{
- MOZ_ASSERT(ShouldReportErrors());
+ MOZ_ASSERT(ShouldReportErrors(mSheet, mLoader));
if (mError.IsEmpty()) {
mError = aErrorText;
} else {
mError.AppendLiteral(" ");
mError.Append(aErrorText);
}
}
void
ErrorReporter::ReportUnexpected(const char *aMessage)
{
- MOZ_ASSERT(ShouldReportErrors());
+ MOZ_ASSERT(ShouldReportErrors(mSheet, mLoader));
nsAutoString str;
sStringBundle->GetStringFromName(aMessage, str);
AddToError(str);
}
void
ErrorReporter::ReportUnexpectedUnescaped(const char *aMessage,
const nsAutoString& aParam)
{
- MOZ_ASSERT(ShouldReportErrors());
+ MOZ_ASSERT(ShouldReportErrors(mSheet, mLoader));
const char16_t *params[1] = { aParam.get() };
nsAutoString str;
sStringBundle->FormatStringFromName(aMessage, params, ArrayLength(params),
str);
AddToError(str);
}
--- a/layout/style/ErrorReporter.h
+++ b/layout/style/ErrorReporter.h
@@ -17,34 +17,37 @@ class nsIURI;
namespace mozilla {
class StyleSheet;
namespace css {
class Loader;
-class ErrorReporter {
+// FIXME(emilio): Probably better to call this ErrorBuilder or something?
+class MOZ_STACK_CLASS ErrorReporter final
+{
public:
ErrorReporter(const StyleSheet* aSheet,
const Loader* aLoader,
nsIURI* aURI);
+
~ErrorReporter();
static void ReleaseGlobals();
static void EnsureGlobalsInitialized()
{
if (MOZ_UNLIKELY(!sInitialized)) {
InitGlobals();
}
}
static bool ShouldReportErrors(const nsIDocument&);
-
- bool ShouldReportErrors();
+ static bool ShouldReportErrors(const StyleSheet* aSheet,
+ const Loader* aLoader);
void OutputError(uint32_t aLineNumber,
uint32_t aLineOffset,
const nsACString& aSource);
void ClearError();
// In all overloads of ReportUnexpected, aMessage is a stringbundle
// name, which will be processed as a format string with the
@@ -59,23 +62,22 @@ public:
private:
void OutputError();
void AddToError(const nsString &aErrorText);
static void InitGlobals();
static bool sInitialized;
static bool sReportErrors;
- nsAutoString mError;
+ nsString mError;
nsString mErrorLine;
nsString mFileName;
const StyleSheet* mSheet;
const Loader* mLoader;
- nsIURI *mURI;
- uint64_t mInnerWindowID;
+ nsIURI* mURI;
uint32_t mErrorLineNumber;
uint32_t mPrevErrorLineNumber;
uint32_t mErrorColNumber;
};
} // namespace css
} // namespace mozilla
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -2772,75 +2772,65 @@ Gecko_SetJemallocThreadLocalArena(bool e
jemalloc_thread_local_arena(enabled);
#endif
}
#include "nsStyleStructList.h"
#undef STYLE_STRUCT
-
-ErrorReporter*
-Gecko_CreateCSSErrorReporter(StyleSheet* aSheet,
- Loader* aLoader,
- nsIURI* aURI)
+bool
+Gecko_ErrorReportingEnabled(const StyleSheet* aSheet, const Loader* aLoader)
{
- MOZ_ASSERT(NS_IsMainThread());
- return new ErrorReporter(aSheet, aLoader, aURI);
+ return ErrorReporter::ShouldReportErrors(aSheet, aLoader);
}
void
-Gecko_DestroyCSSErrorReporter(ErrorReporter* reporter)
-{
- delete reporter;
-}
-
-void
-Gecko_ReportUnexpectedCSSError(ErrorReporter* reporter,
+Gecko_ReportUnexpectedCSSError(const StyleSheet* aSheet,
+ const Loader* aLoader,
+ nsIURI* aURI,
const char* message,
const char* param,
uint32_t paramLen,
const char* prefix,
const char* prefixParam,
uint32_t prefixParamLen,
const char* suffix,
const char* source,
uint32_t sourceLen,
uint32_t lineNumber,
uint32_t colNumber)
{
- if (!reporter->ShouldReportErrors()) {
- return;
- }
-
MOZ_RELEASE_ASSERT(NS_IsMainThread());
+ ErrorReporter reporter(aSheet, aLoader, aURI);
+
if (prefix) {
if (prefixParam) {
nsDependentCSubstring paramValue(prefixParam, prefixParamLen);
nsAutoString wideParam = NS_ConvertUTF8toUTF16(paramValue);
- reporter->ReportUnexpectedUnescaped(prefix, wideParam);
+ reporter.ReportUnexpectedUnescaped(prefix, wideParam);
} else {
- reporter->ReportUnexpected(prefix);
+ reporter.ReportUnexpected(prefix);
}
}
if (param) {
nsDependentCSubstring paramValue(param, paramLen);
nsAutoString wideParam = NS_ConvertUTF8toUTF16(paramValue);
- reporter->ReportUnexpectedUnescaped(message, wideParam);
+ reporter.ReportUnexpectedUnescaped(message, wideParam);
} else {
- reporter->ReportUnexpected(message);
+ reporter.ReportUnexpected(message);
}
if (suffix) {
- reporter->ReportUnexpected(suffix);
+ reporter.ReportUnexpected(suffix);
}
nsDependentCSubstring sourceValue(source, sourceLen);
- reporter->OutputError(lineNumber, colNumber, sourceValue);
+ reporter.OutputError(lineNumber, colNumber, sourceValue);
}
void
Gecko_AddBufferToCrashReport(const void* addr, size_t len)
{
MOZ_ASSERT(NS_IsMainThread());
nsCOMPtr<nsICrashReporter> cr = do_GetService("@mozilla.org/toolkit/crash-reporter;1");
NS_ENSURE_TRUE_VOID(cr);
--- a/layout/style/ServoBindings.h
+++ b/layout/style/ServoBindings.h
@@ -681,21 +681,21 @@ void Gecko_AnnotateCrashReport(const cha
const uint32_t SERVO_CSS_PSEUDO_ELEMENT_FLAGS_##name_ = flags_;
#include "nsCSSPseudoElementList.h"
#undef CSS_PSEUDO_ELEMENT
#define SERVO_BINDING_FUNC(name_, return_, ...) return_ name_(__VA_ARGS__);
#include "mozilla/ServoBindingList.h"
#undef SERVO_BINDING_FUNC
-mozilla::css::ErrorReporter* Gecko_CreateCSSErrorReporter(mozilla::StyleSheet* sheet,
- mozilla::css::Loader* loader,
- nsIURI* uri);
-void Gecko_DestroyCSSErrorReporter(mozilla::css::ErrorReporter* reporter);
-void Gecko_ReportUnexpectedCSSError(mozilla::css::ErrorReporter* reporter,
+bool Gecko_ErrorReportingEnabled(const mozilla::StyleSheet* sheet,
+ const mozilla::css::Loader* loader);
+void Gecko_ReportUnexpectedCSSError(const mozilla::StyleSheet* sheet,
+ const mozilla::css::Loader* loader,
+ nsIURI* uri,
const char* message,
const char* param,
uint32_t paramLen,
const char* prefix,
const char* prefixParam,
uint32_t prefixParamLen,
const char* suffix,
const char* source,
--- a/layout/style/ServoBindings.toml
+++ b/layout/style/ServoBindings.toml
@@ -62,17 +62,16 @@ headers = [
"mozilla/dom/MediaList.h",
"mozilla/dom/ShadowRoot.h",
"mozilla/AnimationPropertySegment.h",
"mozilla/ComputedTiming.h",
"mozilla/ComputedTimingFunction.h",
"mozilla/Keyframe.h",
"mozilla/ServoElementSnapshot.h",
"mozilla/ServoElementSnapshotTable.h",
- "mozilla/css/ErrorReporter.h",
"mozilla/dom/Element.h",
"mozilla/dom/ChildIterator.h",
"mozilla/dom/NameSpaceConstants.h",
"mozilla/LookAndFeel.h",
"mozilla/StaticPrefs.h",
"mozilla/ServoBindings.h",
"mozilla/ComputedStyle.h",
"mozilla/ServoTraversalStatistics.h",
@@ -219,17 +218,16 @@ whitelist-types = [
"mozilla::ComputedTimingFunction::BeforeFlag",
"mozilla::SeenPtrs",
"mozilla::ServoElementSnapshot.*",
"mozilla::ComputedStyle",
"mozilla::StyleSheet",
"mozilla::ServoStyleSheetInner",
"mozilla::ServoStyleSetSizes",
"mozilla::ServoTraversalStatistics",
- "mozilla::css::ErrorReporter",
"mozilla::css::LoaderReusableStyleSheets",
"mozilla::css::SheetLoadData",
"mozilla::css::SheetLoadDataHolder",
"mozilla::css::SheetParsingMode",
"mozilla::css::URLMatchingFunction",
"mozilla::dom::IterationCompositeOperation",
"mozilla::dom::StyleChildrenIterator",
"mozilla::HalfCorner",
@@ -448,17 +446,16 @@ raw-lines = [
"pub type ComputedStyleBorrowed<'a> = &'a ::properties::ComputedValues;",
"pub type ComputedStyleBorrowedOrNull<'a> = Option<&'a ::properties::ComputedValues>;",
"pub type ServoComputedDataBorrowed<'a> = &'a ServoComputedData;",
"pub type RawServoAnimationValueTableBorrowed<'a> = &'a ();"
]
whitelist-functions = ["Servo_.*", "Gecko_.*"]
structs-types = [
"mozilla::css::GridTemplateAreasValue",
- "mozilla::css::ErrorReporter",
"mozilla::css::ImageValue",
"mozilla::css::URLValue",
"mozilla::css::URLValueData",
"mozilla::dom::CallerType",
"mozilla::dom::ShadowRoot",
"mozilla::AnonymousCounterStyle",
"mozilla::AtomArray",
"mozilla::FontStretch",
--- a/modules/libpref/init/StaticPrefList.h
+++ b/modules/libpref/init/StaticPrefList.h
@@ -165,16 +165,23 @@ VARCACHE_PREF(
// Is parallel CSS parsing enabled?
VARCACHE_PREF(
"layout.css.parsing.parallel",
layout_css_parsing_parallel,
bool, true
)
+// Is CSS error reporting enabled?
+VARCACHE_PREF(
+ "layout.css.report_errors",
+ layout_css_report_errors,
+ bool, true
+)
+
// Is support for the font-display @font-face descriptor enabled?
VARCACHE_PREF(
"layout.css.font-display.enabled",
layout_css_font_display_enabled,
bool, true
)
// Are webkit-prefixed properties & property-values supported?
--- a/servo/ports/geckolib/error_reporter.rs
+++ b/servo/ports/geckolib/error_reporter.rs
@@ -4,52 +4,54 @@
//! Wrapper around Gecko's CSS error reporting mechanism.
#![allow(unsafe_code)]
use cssparser::{CowRcStr, serialize_identifier, ToCss};
use cssparser::{SourceLocation, ParseError, ParseErrorKind, Token, BasicParseErrorKind};
use selectors::parser::SelectorParseErrorKind;
+use std::cell::Cell;
use std::ffi::CStr;
use std::ptr;
use style::error_reporting::{ParseErrorReporter, ContextualParseError};
-use style::gecko_bindings::bindings::{Gecko_CreateCSSErrorReporter, Gecko_DestroyCSSErrorReporter};
-use style::gecko_bindings::bindings::Gecko_ReportUnexpectedCSSError;
+use style::gecko_bindings::bindings;
use style::gecko_bindings::structs::{Loader, StyleSheet as DomStyleSheet, nsIURI};
-use style::gecko_bindings::structs::ErrorReporter as GeckoErrorReporter;
use style::gecko_bindings::structs::URLExtraData as RawUrlExtraData;
use style::stylesheets::UrlExtraData;
use style_traits::{StyleParseErrorKind, ValueParseErrorKind};
pub type ErrorKind<'i> = ParseErrorKind<'i, StyleParseErrorKind<'i>>;
-/// Wrapper around an instance of Gecko's CSS error reporter.
-pub struct ErrorReporter(*mut GeckoErrorReporter);
+/// An error reporter with all the data we need to report errors.
+pub struct ErrorReporter {
+ sheet: *const DomStyleSheet,
+ loader: *const Loader,
+ uri: *mut nsIURI,
+ cached_error_reporting_enabled: Cell<Option<bool>>,
+}
impl ErrorReporter {
/// Create a new instance of the Gecko error reporter.
pub fn new(
sheet: *mut DomStyleSheet,
loader: *mut Loader,
extra_data: *mut RawUrlExtraData,
) -> Self {
- unsafe {
- let url = extra_data.as_ref()
+ let uri = unsafe {
+ extra_data.as_ref()
.map(|d| d.mBaseURI.raw::<nsIURI>())
- .unwrap_or(ptr::null_mut());
- ErrorReporter(Gecko_CreateCSSErrorReporter(sheet, loader, url))
- }
- }
-}
+ .unwrap_or(ptr::null_mut())
+ };
-impl Drop for ErrorReporter {
- fn drop(&mut self) {
- unsafe {
- Gecko_DestroyCSSErrorReporter(self.0);
+ ErrorReporter {
+ sheet,
+ loader,
+ uri,
+ cached_error_reporting_enabled: Cell::new(None),
}
}
}
enum ErrorString<'a> {
Snippet(CowRcStr<'a>),
Ident(CowRcStr<'a>),
UnexpectedToken(Token<'a>),
@@ -386,45 +388,64 @@ impl<'a> ErrorHelpers<'a> for Contextual
}
}
};
(None, msg, action)
}
}
impl ErrorReporter {
+ fn reporting_enabled(&self) -> bool {
+ if let Some(enabled) = self.cached_error_reporting_enabled.get() {
+ return enabled;
+ }
+ let enabled = unsafe {
+ bindings::Gecko_ErrorReportingEnabled(self.sheet, self.loader)
+ };
+ self.cached_error_reporting_enabled.set(Some(enabled));
+ enabled
+ }
+
pub fn report(&self, location: SourceLocation, error: ContextualParseError) {
+ if !self.reporting_enabled() {
+ return;
+ }
+
let (pre, name, action) = error.to_gecko_message();
let suffix = match action {
Action::Nothing => ptr::null(),
Action::Skip => cstr!("PEDeclSkipped").as_ptr(),
Action::Drop => cstr!("PEDeclDropped").as_ptr(),
};
let params = error.error_params();
let param = params.main_param;
let pre_param = params.prefix_param;
let param = param.map(|p| p.into_str());
let pre_param = pre_param.map(|p| p.into_str());
let param_ptr = param.as_ref().map_or(ptr::null(), |p| p.as_ptr());
let pre_param_ptr = pre_param.as_ref().map_or(ptr::null(), |p| p.as_ptr());
// The CSS source text is unused and will be removed in bug 1381188.
let source = "";
unsafe {
- Gecko_ReportUnexpectedCSSError(self.0,
- name.as_ptr() as *const _,
- param_ptr as *const _,
- param.as_ref().map_or(0, |p| p.len()) as u32,
- pre.map_or(ptr::null(), |p| p.as_ptr()) as *const _,
- pre_param_ptr as *const _,
- pre_param.as_ref().map_or(0, |p| p.len()) as u32,
- suffix as *const _,
- source.as_ptr() as *const _,
- source.len() as u32,
- location.line,
- location.column);
+ bindings::Gecko_ReportUnexpectedCSSError(
+ self.sheet,
+ self.loader,
+ self.uri,
+ name.as_ptr() as *const _,
+ param_ptr as *const _,
+ param.as_ref().map_or(0, |p| p.len()) as u32,
+ pre.map_or(ptr::null(), |p| p.as_ptr()) as *const _,
+ pre_param_ptr as *const _,
+ pre_param.as_ref().map_or(0, |p| p.len()) as u32,
+ suffix as *const _,
+ source.as_ptr() as *const _,
+ source.len() as u32,
+ location.line,
+ location.column,
+ );
}
}
}
impl ParseErrorReporter for ErrorReporter {
fn report_error(
&self,
_url: &UrlExtraData,