Errors in the error console with very long reference lines hang the browser

RESOLVED FIXED in mozilla21

Status

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
mozilla21
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Just like bug 796179. If there is a CSS file that has had line-breaks removed so all CSS is on the same line, we try writing out the whole line to the error console to describe where the CSS syntax error is.

This can (and does) hang the browser while it tries to lay out all of the text.

It would be great if we could trim the string surrounding the actual syntax error. Trimming from the beginning of the string will be simple, but won't provide any value to the developer.
See Also: → 796179
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Posted patch Patch (obsolete) — Splinter Review
This patch implements the behavior that I described in comment #0.

It will truncate the text surrounding the column of the error if the string is too long.

I tried to remove some duplication between this and the href truncating, but hopefully it doesn't make the code too hard to follow.
Attachment #702561 - Flags: review?(rcampbell)
Comment on attachment 702561 [details] [diff] [review]
Patch

There's a bug here where "href" is getting set to undefined. Investigating...
Attachment #702561 - Flags: review?(rcampbell)
Posted patch Patch v1.1 (obsolete) — Splinter Review
Fixed issue with href getting set to undefined.
Attachment #702561 - Attachment is obsolete: true
Attachment #702575 - Flags: review?(rcampbell)
Duplicate of this bug: 832614
Attachment #702575 - Flags: review?(neil)
I don't want to scope-creep this bug, but it looks like there's a bunch of formatting work being done as each message arrives, even if the console window isn't open.  This is wasted effort in the (overwhelmingly likely) case that the console window never gets opened.  In the specific case of errors coming from the CSS parser, I think it also means that we don't actually get the benefits of sharing the nsString for all nsIScriptError objects pointing to the same source line.

How hard would it be to delay the creation of console rows if the window isn't open, instead just hanging onto all the message objects?
Also, as a comment on *this* bug, I think it would be better to show less of the line to the left of the error position, so that the error position is more likely to be visible at the default width of the error console.  Concretely, something like "if the error column is > 80 then truncate the line to position the error at column 40".
(In reply to Zack Weinberg from comment #6)
> I don't want to scope-creep this bug, but it looks like there's a bunch of
> formatting work being done as each message arrives, even if the console
> window isn't open.
No, consoleBindings.xml is part of the console window.
Comment on attachment 702575 [details] [diff] [review]
Patch v1.1

I think it might possibly be clearer to divide the work up according to aMiddleCharacter:
if (!aMiddleCharacter || aMiddleCharacter < halfLimit)
  return [the beginning of the string]
if (aMiddleCharacter < aString.length - halfLimit)
  return [the characters around aMiddleCharacter]
return [the end of the string]

>+        <parameter name="aLimit"/>
[Is aLimit always going to be fieldMaxLength?]

>+            aMiddleCharacter = aLimit / 2;
You use aLimit / 2 a lot! Luckily it's even :-)

>+            truncatedString = "…" + truncatedString;
>+          if (endPosition < aString.length)
>+            truncatedString = truncatedString + "…";
[I wonder whether it's worth using the intl.ellipsis pref]

>+            column: aMiddleCharacter - startPosition,
Doesn't account for the ellipsis that you prepended to the string segment.

>-          let anyMatch = ["msg", "line", "code"].some(function (key) {
>+          let anyMatch = ["msg", "code"].some(function (key) {
>             return (aRow.hasAttribute(key) &&
>                     this.stringMatchesFilters(aRow.getAttribute(key), this.mFilter));
>           }, this)
This is now almost as long as writing it out longhand ;-)
(In reply to neil@parkwaycc.co.uk from comment #8)
> (In reply to Zack Weinberg from comment #6)
> > I don't want to scope-creep this bug, but it looks like there's a bunch of
> > formatting work being done as each message arrives, even if the console
> > window isn't open.
> No, consoleBindings.xml is part of the console window.

Ah, I misunderstood the initialization here as happening on startup rather than when the window was opened.  Sorry for the noise.
Posted patch Patch v1.2Splinter Review
(In reply to neil@parkwaycc.co.uk from comment #9)
> I think it might possibly be clearer to divide the work up according to
> aMiddleCharacter:
> if (!aMiddleCharacter || aMiddleCharacter < halfLimit)
>   return [the beginning of the string]
> if (aMiddleCharacter < aString.length - halfLimit)
>   return [the characters around aMiddleCharacter]
> return [the end of the string]

I tried doing it this way but the code ended up being longer, harder for me to follow.
 
> >+        <parameter name="aLimit"/>
> [Is aLimit always going to be fieldMaxLength?]

Yeah, I removed this argument and just referenced this.fieldMaxLength now.
 
> >+            aMiddleCharacter = aLimit / 2;
> You use aLimit / 2 a lot! Luckily it's even :-)

Moved to a local variable, halfLimit, like you have in your sample code.

> >+            truncatedString = "…" + truncatedString;
> >+          if (endPosition < aString.length)
> >+            truncatedString = truncatedString + "…";
> [I wonder whether it's worth using the intl.ellipsis pref]
> >+            column: aMiddleCharacter - startPosition,
> Doesn't account for the ellipsis that you prepended to the string segment.

Fixed, thanks.

> >-          let anyMatch = ["msg", "line", "code"].some(function (key) {
> >+          let anyMatch = ["msg", "code"].some(function (key) {
> >             return (aRow.hasAttribute(key) &&
> >                     this.stringMatchesFilters(aRow.getAttribute(key), this.mFilter));
> >           }, this)
> This is now almost as long as writing it out longhand ;-)

Fixed.
Attachment #702575 - Attachment is obsolete: true
Attachment #702575 - Flags: review?(rcampbell)
Attachment #702575 - Flags: review?(neil)
Attachment #705141 - Flags: review?(neil)
Comment on attachment 705141 [details] [diff] [review]
Patch v1.2

>+          let startPosition = 0;
>+          let endPosition = aString.length;
>+          if (aMiddleCharacter - halfLimit >= 0)
>+            startPosition = aMiddleCharacter - halfLimit;
>+          if (aMiddleCharacter + halfLimit <= aString.length)
>+            endPosition = aMiddleCharacter + halfLimit;
So if aMiddleCharacter is, say, 1, then you're going to return the first halfLimit + 1 characters of the string. Is that reasonable? (The alternative would be to return the first fieldMaxLength characters of the string.)

>+            truncatedString = ellipsis + truncatedString;
>+            aMiddleCharacter += 1;
[Nit: += ellipsis.length]
(In reply to comment #12)
> >+          let startPosition = 0;
> >+          let endPosition = aString.length;
> >+          if (aMiddleCharacter - halfLimit >= 0)
> >+            startPosition = aMiddleCharacter - halfLimit;
> >+          if (aMiddleCharacter + halfLimit <= aString.length)
> >+            endPosition = aMiddleCharacter + halfLimit;
> So if aMiddleCharacter is, say, 1, then you're going to return the first
> halfLimit + 1 characters of the string. Is that reasonable? (The alternative
> would be to return the first fieldMaxLength characters of the string.)

Oh, and it dawns on me that the easy way to change this might be this:
start = constrain (middle - limit / 2) between 0 and (aString.length - limit)
string = slice string, start, limit
Bah, I can't test this with a JS error because jsexn.cpp automatically limits the error source code string to 120 characters...
Oh, I guess I could patch the limit to 50 locally for test purposes.
(In reply to comment #14)
> Bah, I can't test this with a JS error because jsexn.cpp automatically
> limits the error source code string to 120 characters...

And it uses a 60-each-side window, which is similar to what you're doing here except with 100 characters, so you can ignore comment 13.
Or test it like this:

data:text/html,<style>.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx{background:notacolor}.yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy{background:alsonotacolor}</style>
Comment on attachment 705141 [details] [diff] [review]
Patch v1.2

>+          let startPosition = 0;
>+          let endPosition = aString.length;
>+          if (aMiddleCharacter - halfLimit >= 0)
>+            startPosition = aMiddleCharacter - halfLimit;
>+          if (aMiddleCharacter + halfLimit <= aString.length)
>+            endPosition = aMiddleCharacter + halfLimit;
>+          let truncatedString = aString.substring(startPosition, endPosition);
As an aside, I looked up the behaviour of substring because I can never remember how it differs from substr and slice. It seems that it already limits its parameters to the start and end of the string, so you don't actually need to limit them to 0 and aString.length manually. (This would mean changing the check to startPosition > 0 below.)

>+            aMiddleCharacter += 1;
[Reminder: += ellipsis.length]

>+            column: aMiddleCharacter - startPosition,
>+          };
[I'm sure the trailing comma used to be a warning at some point...]
Attachment #705141 - Flags: review?(neil) → review+
I was *just* getting to this. Thanks for reviewing, Neil and thanks for doing this Jared.
https://hg.mozilla.org/mozilla-central/rev/7c5ab9bc4a5d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Query, should jsexn.cpp stop windowing text now, or should the CSS error reporter start?  I'm happy to file whatever follow-up bug is considered appropriate.  It seems to me that both should behave the same way (if windowing before rendering is desired, perhaps it should happen in ns(I)ScriptError?)
(In reply to Zack Weinberg (:zwol) from comment #22)
> Query, should jsexn.cpp stop windowing text now, or should the CSS error
> reporter start?  I'm happy to file whatever follow-up bug is considered
> appropriate.  It seems to me that both should behave the same way (if
> windowing before rendering is desired, perhaps it should happen in
> ns(I)ScriptError?)

Windowing in the front end is better since that will provide consumers better ability to search the strings, as long as it is not too expensive to pass the memory around.

Updated

6 years ago
Depends on: 841712
See Also: → 877773
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.