Bug 111133 – Long URLs in error messages should be shortened

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Long URLs in error messages should be shortened

.... (10MB of data)....
XVpLTMiPjwvbGk+Ci0tPgoKIAo=' because the document's frame is sandboxed and the 'allow-scripts' permission is not set.

I'm sure we have lots of these. :(  I suspect this is affecting performance of some of our data: url tests (including perf tests).

[Mike West]

We're already visually shortening the messages in the console. I don't see any reason that we couldn't shorten the actual string when it's over some arbitrarily large limit. We'd lose the ability to click through to see the page that generated the error, but for this specific case that's probably not a terrible loss.

1. Can you point me at a test result that's currently shockingly large?
2. What's the most performant mechanism to shorten a string? :)

[Eric Seidel (no email)]

Oh, the log that I'm seeing is not on the console, it's to stdout/stderr. :)  I see this when running DRT manually.

[Mike West]

Right, we don't shorten the string at all, we just look for URLs and elide them when they're over some length (~150 characters, I think). It's perfectly reasonable to do that at the string level for some higher limit (1k?). I'll throw up a patch as soon as I remember how to export methods from KURL for use in WebCore. :/

[Mike West]

(In reply to comment #3)
> I'll throw up a patch as soon as I remember how to export methods from KURL for use in WebCore. :/

Scratch that. I'll upload a patch as soon as I realize that I'm an idiot who can't properly read linker errors. :)

[Eric Seidel (no email)]

I think we need to do a somewhat careful audit of all the times we're copying (see bug 111134) or converting urls to strings.  URLs can literally be megabytes in length in WebKit.  :)

[Eric Seidel (no email)]

(I'm not suggesting we need to do such as part of this bug.  More just complaining in public.)

[Mike West]

Created attachment 190916 [details]
Patch

[Mike West]

(In reply to comment #7)
> Created an attachment (id=190916) [details]
> Patch

Feedback welcome, Eric and Pavel. If this seems like a workable approach, I'll go through the rest of the console messages that contain URLs and add them to the patch.

I'm a bit worried about the substring and concat approach in terms of performance. I'd appreciate alternative suggestions.

[Eric Seidel (no email)]

Comment on attachment 190916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190916&action=review

> Source/WebCore/bindings/ScriptControllerBase.cpp:45
> +            m_frame->document()->addConsoleMessage(HTMLMessageSource, ErrorMessageLevel, "Blocked script execution in '" + m_frame->document()->url().elidedString() + "' because the document's frame is sandboxed and the 'allow-scripts' permission is not set."); 

I wonder if we should be ASSERTing in addConsoleMessage for certain error levels that we're not above a certain size threshold.

[Eric Seidel (no email)]

Going to leave this for abarth or pfeldman to review.  I'm a bit too tired to be a good reviewer right now. :)

[Mike West]

(In reply to comment #9)
> (From update of attachment 190916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190916&action=review
> 
> > Source/WebCore/bindings/ScriptControllerBase.cpp:45
> > +            m_frame->document()->addConsoleMessage(HTMLMessageSource, ErrorMessageLevel, "Blocked script execution in '" + m_frame->document()->url().elidedString() + "' because the document's frame is sandboxed and the 'allow-scripts' permission is not set."); 
> 
> I wonder if we should be ASSERTing in addConsoleMessage for certain error levels that we're not above a certain size threshold.

That probably makes sense for some reasonably high amount. 10k? 100k?

Pavel will certainly have opinions here. :)

[Mike West]

Jochen expressed some doubt about putting this on KURL. Adding him in case his doubt grows into an r-.

[Adam Barth]

Comment on attachment 190916 [details]
Patch

LGTM, but I'll give jochen and others a chance to comment.

Comment on attachment 190916 [details]
Patch

ok

(In reply to comment #12)
> Jochen expressed some doubt about putting this on KURL. Adding him in case his doubt grows into an r-.

I thought you were talking about a case where the inspector is passed the URL. But you're passing a string, so you'll have to do the conversion earlier, so it's ok

[Mike West]

(In reply to comment #15)
> (In reply to comment #12)
> > Jochen expressed some doubt about putting this on KURL. Adding him in case his doubt grows into an r-.
> 
> I thought you were talking about a case where the inspector is passed the URL. But you're passing a string, so you'll have to do the conversion earlier, so it's ok

Cool. I'll add a few of the more obvious cases of copying full, potentially long URLs and land this on Monday.

We can take a closer look at the non-obvious bits in bug 111134.

[Mike West]

Created attachment 191115 [details]
Patch

[Mike West]

Comment on attachment 191115 [details]
Patch

Clearing r?, carrying over Jochen's r+. I'll wait until Monday to land this.

[WebKit Review Bot]

Comment on attachment 191115 [details]
Patch

Clearing flags on attachment: 191115

Committed r144607: <http://trac.webkit.org/changeset/144607>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 191115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191115&action=review

> Source/WebCore/ChangeLog:28
> +        * platform/KURL.cpp:
> +        (WebCore::KURL::elidedString): Added.
> +        * platform/KURL.h:
> +            An exciting new method that gives you the same result as string()
> +            for URLs less than 1k long, and elides the middle of URLs longer
> +            than 1k by replacing everything but the first and last 0.5k with
> +            "...".

The name “elided string” does not seem good for this function.

It’s nice that it’s a short name. But the string is not “elided”, only the middle of the string is.

[Mike West]

(In reply to comment #21)
> (From update of attachment 191115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191115&action=review
> 
> > Source/WebCore/ChangeLog:28
> > +        * platform/KURL.cpp:
> > +        (WebCore::KURL::elidedString): Added.
> > +        * platform/KURL.h:
> > +            An exciting new method that gives you the same result as string()
> > +            for URLs less than 1k long, and elides the middle of URLs longer
> > +            than 1k by replacing everything but the first and last 0.5k with
> > +            "...".
> 
> The name “elided string” does not seem good for this function.
> 
> It’s nice that it’s a short name. But the string is not “elided”, only the middle of the string is.

Hrm. I thought the name fit well conceptually, but I'm happy to change it for clarity.

"ellipsisedString"? I'm not sure "ellipsised" is a word. :) "stringTrimmedTo1k"? It looks like DevTools has a 'trimMiddle' function[1], perhaps following that convention would be reasonable?

[1]: http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/utilities.js#L117

[Mike West]

Split out renaming into https://bugs.webkit.org/show_bug.cgi?id=111700.