Closed Bug 1235159 Opened 7 years ago Closed 6 years ago

Report pattern attribute compilation failure to Web Console.


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox46 --- fixed


(Reporter: arai, Assigned: arai)




(1 file, 1 obsolete file)

Derived from bug 1227906.

Now compilation failure for pattern attribute is reported only to Browser Console, without any other context, for example, file is "<unknown>:1:0".
It should be better to report it to the tab's Web Console, with some more context (maybe a pattern string?)

Also, bug 1227906 will add "u" flag for the RegExp compilation, and it will reject some previously-working patterns.  So reporting it to Web Console should help web page authors.

See also
This is based on bug 1227906.
and I guess these 2 bugs should be fixed at the same time, or at least in same cycle.

Added error reporting to Web Console when the "pattern" attribute compilation fails.
ReportPatternCompileFailure extracts error message from the pending exception, and report it with passed pattern string, to make it debuggable.
Assignee: nobody → arai.unmht
Attachment #8704921 - Flags: review?(jst)
Comment on attachment 8704921 [details] [diff] [review]
Report pattern compliation failure to web console.

+    const char16_t* pattern = ToNewUnicode(aPattern);
+    const char16_t *strings[] = { pattern, message };
+    nsContentUtils::ReportToConsole(nsIScriptError::errorFlag,
+                                    NS_LITERAL_CSTRING("DOM"),
+                                    aDocument,
+                                    nsContentUtils::eDOM_PROPERTIES,
+                                    "PatternAttributeCompileFailure",
+                                    strings, ArrayLength(strings));
+    free((void*)pattern);

Maybe use PromiseFlatString here to avoid havin to manually free pattern?

- In dom/locales/en-US/chrome/dom/

+PatternAttributeCompileFailure=Failed to compile pattern attribute '%S': %S
\ No newline at end of file

Add a newline at the end of the file.

r=jst, but requesting review from jorendorff to get some JS eyeballs on all the value rooting etc.
Attachment #8704921 - Flags: review?(jorendorff)
thank you for reviewing :)

addressed review comments, and flipped wrong assertion conditions.
Attachment #8704921 - Attachment is obsolete: true
Attachment #8704921 - Flags: review?(jst)
Attachment #8704921 - Flags: review?(jorendorff)
Attachment #8706725 - Flags: review?(jorendorff)
Comment on attachment 8706725 [details] [diff] [review]
Report pattern compliation failure to web console. r=jst

Review of attachment 8706725 [details] [diff] [review]:

::: dom/base/nsContentUtils.cpp
@@ +6635,5 @@
>  }
> +static bool
> +ReportPatternCompileFailure(nsAString& aPattern, nsIDocument* aDocument,
> +                            JSContext* cx)

This should be a void function.

On failure, it should leave the exception state of cx unchanged.

On success, it should clear the exception so that the AutoJSAPI does not also automatically report it.

@@ +6641,5 @@
> +    MOZ_ASSERT(JS_IsExceptionPending(cx));
> +
> +    JS::RootedValue exn(cx);
> +    if (!JS_GetPendingException(cx, &exn)) {
> +      return false;

just `return;`

@@ +6645,5 @@
> +      return false;
> +    }
> +    if (!exn.isObject()) {
> +      // If pending exception is not an object, it should be OOM.
> +      return false;

same here

@@ +6652,5 @@
> +    JS::AutoSaveExceptionState savedExc(cx);
> +    JS::RootedObject exnObj(cx, &exn.toObject());
> +    JS::RootedValue messageVal(cx);
> +    if (!JS_GetProperty(cx, exnObj, "message", &messageVal)) {
> +      return false;

Failing here is impossible, right? If so, just `return;`.

But if it's possible this can fail, then what you want to do is


because it is (slightly) better to have AutoJSAPI report the initial exception than to discard it and report the new problem instead.

@@ +6663,5 @@
> +    if (!flatMessage) {
> +      return false;
> +    }
> +
> +    JS::AutoCheckCannotGC nogc;

This is no good. nsContentUtils::ReportToConsole can GC.

Anyway, there's an easier way to make sure the string is converted to char16_t:

    nsAutoString wideMessage;
    if (!AssignJSString(cx, wideMessage, messageStr))

(AssignJSString is defined in nsJSUtils.h.)

You can drop the call to JS_FlattenString, too.

@@ +6685,5 @@
> +                                    nsContentUtils::eDOM_PROPERTIES,
> +                                    "PatternAttributeCompileFailure",
> +                                    strings, ArrayLength(strings));
> +    return true;
> +}

At the end, savedExc.drop(). Because we have reported the error, we do not want savedExc's destructor to re-raise it.

@@ +6720,5 @@
> +    aPattern.Cut(0, 4);
> +    aPattern.Cut(aPattern.Length() - 2, 2);
> +    if (!ReportPatternCompileFailure(aPattern, aDocument, cx)) {
> +      return false;
> +    }

A comment above says:
  // Failure to create or run the regexp results in the invalid pattern
  // matching, but we can still report the error to the console.

Note that any error we do not explicitly clear will be reported by the AutoJSAPI.

So ReportPatternCompileFailure() should be a void function, and we should unconditionally return true after calling it.

::: dom/locales/en-US/chrome/dom/
@@ +188,5 @@
>  InterceptionRejectedResponseWithURL=Failed to load '%1$S'. A ServiceWorker passed a promise to FetchEvent.respondWith() that rejected with '%2$S'.
>  # LOCALIZATION NOTE: Do not translate "ServiceWorker", "promise", "FetchEvent.respondWith()", or "Response". %1$S is a URL. %2$S is an error string.
>  InterceptedNonResponseWithURL=Failed to load '%1$S'. A ServiceWorker passed a promise to FetchEvent.respondWith() that resolved with non-Response value '%2$S'.
>  ExecCommandCutCopyDeniedNotInputDriven=document.execCommand('cut'/'copy') was denied because it was not called from inside a short running user-generated event handler.
> +PatternAttributeCompileFailure=Failed to compile pattern attribute '%S': %S

The words "compile" and "attribute" are not necessarily helpful here, and I think it would be good to mention that we expect a regular expression. How about:

    Unable to check <input pattern='%S'> because the pattern is not a valid regexp: %S

That is a lot longer. But I think it's OK?

I'm under the impression that <input pattern=> is the only place where patterns can appear in HTML. If that's not true, then you'd want something more like:

    Unable to check pattern '%S' because it is not a valid regexp: %S
Attachment #8706725 - Flags: review?(jorendorff) → review+
Bug 1235159 - Report pattern compliation failure to web console. r=jst,jorendorff
Bug 1235159 - followup: Fix test_bug1112040.html to check updated message. r=bustage
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.