1493361 - URLs reported to the console can be overly long

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[(no longer active)]

Compare the two output, one is before the patch, the other is after:

Request to access cookie or storage on “https://googleads.g.doubleclick.net/pagead/ads?client=ca-pub-8548565564007782&output=html&h=364&slotname=3808230426&adk=2963870208&adf=3135775153&w=608&cr_col=4&cr_row=2&fwrn=2&lmt=1537590956&pucrd=CgwIARAAGAAgACgAOAF4Aw&rafmt=9&guci=2.2.0.0.2.2.0&format=608x364&url=https%3A%2F%2Fwww.whatismybrowser.com%2Fdetect%2Fare-third-party-cookies-enabled&flash=0&crui=image_stacked&fwr=0&wgl=1&adsid=NT&dt=1537590956284&bpp=7&bdt=403&fdt=15&idt=250&shv=r20180917&cbv=r20180604&saldr=aa&abxe=1&correlator=1353440111225&frm=20&pv=2&ga_vid=1816646895.1537590957&ga_sid=1537590957&ga_hid=1823917154&ga_fc=0&icsg=133760&dssz=17&mdo=0&mso=0&u_tz=-240&u_his=2&u_java=0&u_h=1080&u_w=1920&u_ah=1052&u_aw=1920&u_cd=24&u_nplug=0&u_nmime=0&adx=474&ady=1564&biw=1908&bih=629&scr_x=0&scr_y=0&eid=21060853%2C26835106%2C828064255&oid=3&rx=0&eae=0&fc=528&brdim=%2C%2C0%2C27%2C1920%2C27%2C1920%2C1025%2C1920%2C629&vis=1&rsz=%7C%7CeEbr%7C&abl=CS&ppjl=t&pfx=0&fu=144&bc=5&jar=2018-9-22-3&ifi=1&xpc=MMiySAwyNe&p=https%3A//www.whatismybrowser.com&dtd=284” was blocked because it came from a tracker and content blocking is enabled.

Request to access cookie or storage on “https://googleads.g.doubleclick.net/pagead/ads?client=ca-pub-8548565564007782&output=html&h=364&slotname=3808230426&adk=29638702…” was blocked because it came from a tracker and content blocking is enabled.

The second one looks much better!

Comment 1

[(no longer active)]

Attachment: Bug 1493361 - Truncate the URLs reported to the console

Comment 2

[Thomas Wisniewski [:twisniewski]]

Nice! But will we be able to expand the URL to see the whole thing? Otherwise, this might end up defeating the point of logging them in the first place.

Comment 3

[Johann Hofmann [:johannh]]

(In reply to Thomas Wisniewski [:twisniewski] from comment #2)
> Nice! But will we be able to expand the URL to see the whole thing?
> Otherwise, this might end up defeating the point of logging them in the
> first place.

Looking at the patch, no. I'm a bit concerned that users won't be able to distinguish the URLs because they also don't show up in the network tab, but maybe there's no actual use case for distinguishing them...

Comment 4

[Thomas Wisniewski [:twisniewski]]

I can say that I've used the console to distinguish between lengthy URLs before in my webcompat diagnosis sessions, but if others who do more anti-tracking diagnoses think that's not common enough to worry about, that's fine.

Comment 5

[(no longer active)]

Brian, do you happen to know if there is a way for Gecko to report these URLs in a way that console can show them collapsed at first but allow the user to click on them to expand or something like that to provide a better use experience here?  Also do you have any feedback on whether this patch is desirable or not?

I wrote this patch because I was assuming that we don't really have any such options available to us, but I could be wrong.

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 9011172 [details]
Bug 1493361 - Truncate the URLs reported to the console

Andrea Marchesini [:baku] has approved the revision.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to :Ehsan Akhgari from comment #5)
> Brian, do you happen to know if there is a way for Gecko to report these
> URLs in a way that console can show them collapsed at first but allow the
> user to click on them to expand or something like that to provide a better
> use experience here?  Also do you have any feedback on whether this patch is
> desirable or not?
> 
> I wrote this patch because I was assuming that we don't really have any such
> options available to us, but I could be wrong.

We do handle long strings for Console API methods. For instance: `console.log(new Array(10000).join("long"))` shows only part of the string and then you can expand it.

However it looks like we don't wire that up with Console Service messages (like errors or security messages). For instance: `throw new Error(new Array(10000).join("long"))` seems to just truncate the message without a way to expand. Even if we were to wire that up as-is with Console Service messages it wouldn't trim just the URL portion of a message, but rather the entire message (so you'd see for instance):

'Request to access cookie or storage on “https://googleads.g.doubleclick.net/pagead/ads?client=ca-pub-8548565564007782&output=html&h=364&slotname=3808230426&adk=2963870208&adf=3135775153&w=608&cr_col=4&cr_row=2&fwrn=2&lmt=1537590956&pucrd=CgwIARAAGAAgACgAOAF4Aw&rafmt=9&guci=2.2.0.0....'

Which isn't what we want - the "was blocked because it came from a tracker and content blocking is enabled." part would be dropped. I suppose we would change the message text around:

'Request to access cookie or storage was blocked because it came from a tracker and content blocking is enabled. URL: "https://googleads.g.doubleclick.net/pagead/ads?client=ca-pub-8548565564007782&output=html&h=364&slotname=3808230426&adk=2963870208&adf=3135775153&w=608&cr_col=4&cr_row=2&fwrn=2&lmt=1537590956&pucrd=CgwIARAAGAAgACgAOAF4Aw&rafmt=9&guci=2.2.0.0...."'

But that seems like a footgun for future messages and/or translations. So I guess we'd need to either:

1) Split up the console service message into parts and somehow annotate some of them as collapsible (seems like it'd get pretty complex although :baku would know better about the difficulty there)
2) Add a frontend-only feature that detects URLs in Console Service and trims them / allows expansion. It wouldn't give the protocol optimization that we get with longStrings (where we don't need to send the whole string across to frontend until it's requested), but it's no worse than the current situation. Nicolas would know better about the difficulty there.
3) Group together all tracking messages into a collapsible group at the top of the UI (see Bug 1491687) and just render the whole URL since it won't become visible until you open the group.

I suspect we'd either want to do what you've already done here, or some combination of (2) and (3). I want to do (3) anyway as part of de-cluttering the UI so I'm a bit biased towards that.

Comment 8

[(no longer active)]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> We do handle long strings for Console API methods. For instance:
> `console.log(new Array(10000).join("long"))` shows only part of the string
> and then you can expand it.

Cool, didn't know about that!

> 'Request to access cookie or storage was blocked because it came from a
> tracker and content blocking is enabled. URL:
> "https://googleads.g.doubleclick.net/pagead/ads?client=ca-pub-
> 8548565564007782&output=html&h=364&slotname=3808230426&adk=2963870208&adf=313
> 5775153&w=608&cr_col=4&cr_row=2&fwrn=2&lmt=1537590956&pucrd=CgwIARAAGAAgACgAO
> AF4Aw&rafmt=9&guci=2.2.0.0...."'
> 
> But that seems like a footgun for future messages and/or translations.

That seems pretty l10n-hostile.  :-/

> So I
> guess we'd need to either:
> 
> 1) Split up the console service message into parts and somehow annotate some
> of them as collapsible (seems like it'd get pretty complex although :baku
> would know better about the difficulty there)
> 2) Add a frontend-only feature that detects URLs in Console Service and
> trims them / allows expansion. It wouldn't give the protocol optimization
> that we get with longStrings (where we don't need to send the whole string
> across to frontend until it's requested), but it's no worse than the current
> situation. Nicolas would know better about the difficulty there.
> 3) Group together all tracking messages into a collapsible group at the top
> of the UI (see Bug 1491687) and just render the whole URL since it won't
> become visible until you open the group.
> 
> I suspect we'd either want to do what you've already done here, or some
> combination of (2) and (3). I want to do (3) anyway as part of de-cluttering
> the UI so I'm a bit biased towards that.

What I was thinking of was much simpler than the above.  As you will note here, at this level, Gecko is already reporting the URL as a separate string.  So there is no point in doing extra work to separate out that piece of data (as you suggested in #1).  The problem is that nsIScriptError.initWithWindowID() and initWithSourceURI() <https://searchfox.org/mozilla-central/rev/881a3c5664ede5e08ee986d76433bc5c4b5680e6/dom/bindings/nsIScriptError.idl#128> accept just an error string, so nsContentUtils::ReportToConsole has to paste this string into the error message before handing it down to the code that would eventually give the data to console.

What if this data was delivered as the string with the format identifier as well as the params, and the front-end would paste it all together itself?  Then it could check the length of each one of the format strings and if they exceed a certain limit (e.g. 128 chars) it could collapse them by default and build UI to expand them on demand when the user clicks on the ellipsis or something.  And this would work for all consumers of nsContentUtils::ReportToConsole without any modifications.

How does this sound?

And a separate question, should this grand plan block the landing of this patch?

Comment 9

[(no longer active)]

(In reply to Johann Hofmann [:johannh] from comment #3)
> (In reply to Thomas Wisniewski [:twisniewski] from comment #2)
> > Nice! But will we be able to expand the URL to see the whole thing?
> > Otherwise, this might end up defeating the point of logging them in the
> > first place.
> 
> Looking at the patch, no. I'm a bit concerned that users won't be able to
> distinguish the URLs because they also don't show up in the network tab

FWIW this isn't true in the case of third-party cookie blocking, only for blocking slow tracking resources.  (We can choose to handle that case differently here if needed.)

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

> What if this data was delivered as the string with the format identifier as well as the params, and the front-end would paste it all together itself?  Then it could check the length of each one of the format strings and if they exceed a certain limit (e.g. 128 chars) it could collapse them by default and build UI to expand them on demand when the user clicks on the ellipsis or something.  And this would work for all consumers of nsContentUtils::ReportToConsole without any modifications.
> How does this sound?

I think that would something doable. I'd disagree about cropping __everything__ at 128, but for URLs it makes sense.

Comment 11

[:Harald Kirschner :digitarald]

Hitting CNN I get warnings with 2.5k characters – this issue got worse with the cookie access warnings.

Moving to Console, as the cosmetic URL clipping need to happen on the client so we don't remove details from platform.

Comment 12

[(no longer active)]

FWIW I have been holding off landing my patch here in the hopes that something better (along the lines of comment 10) can be done instead...

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

I'm going to give this a try

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1493361 - Add a parameters property to nsIScriptError to handle complex values in the frontend. r=ehsan.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1493361 - Replace nsIScriptError parameters with Reps in the console. r=Honza.

Depends on D28643

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1493361 - Use nsIScriptError's parameters for content blocking messages. r=ehsan.

Depends on D28644

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Ehsan, if you have some time I'd really appreciate feedback and help on the 2 C++ patches attached as it's taking me quite some time to figure things out. Thanks!

Comment 18

[(no longer active)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #17)

Ehsan, if you have some time I'd really appreciate feedback and help on the 2 C++ patches attached as it's taking me quite some time to figure things out. Thanks!

I commented on https://phabricator.services.mozilla.com/D28643. I haven't yet looked at the patches too deeply since your investigations may still change the patches but overall they looked good!

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Actual, cropped, links

Here's how it can looks like.
In this screenshot, URL are cropped at 100 characters. They're also actual URL, so you can click on them top open them, or right-click to copy to clipboard. Hovering a cropped URL gives the full URL as well.

I guess we can discuss where we could crop, as it still feels a bit long to me (we could maybe go to 80?).

Comment 20

[Francesco Lodolo [:flod]]

Using NI, because Phabricator is really not helpful for poking people.

Can you take a look at
https://phabricator.services.mozilla.com/D28646#inline-163996

Comment 21

[Axel Hecht [:Pike]]

Sorry, I don't have good news.

Our supported surface of printf is documented on https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/xpcom/string/nsTextFormatter.h#17-31. What the patch does is to explicitly rely on the details on how that code handles unknown parameters.

To make that at least somewhat stable, we would need to invest into compare-locales to build out printf muscles, and I really don't want to invest into that dying part of our ecosystem. It took us about a decade to get to the current stable state, where mishaps around formatters fall back safely, and introducing formatters that don't would be disappointing.

That said, I think we should at some point have a Fluent-enabled error console, which passes error meta data, fluent resource, and ID all the way to the console service, and generates the localized error message at display time. That would, among other things, allow to retranslate error messages on language switching, but probably also addresses the questions raised here for ways to do more interesting things on display. That's about as concrete as my thinking about this part of our UI and the transition to Fluent is at this point.

I have no idea how close that is to what you'd end up doing anyway, but I'm afraid it's not close.

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Thanks for your answer. I'm definitely not close to what you're suggesting.

We could drop all this platform patch, and use the StringRep to render every error message (so we get the linkification).
What I don't like with this approach is that we parse the message to detect links, which seems silly as in Gecko, we know we want to display URLs.

Also, this patch was meant to ease the work for Bug 1545888, where we could have use the placeholders easily.

Given the timeline of the grouping features we want to have, I'm not sure what to do.
I guess we could keep the property message as is, and still include those URLs as nsIScriptErrorParameter.

Then on the frontend, we won't have to parse the text as much, simply search for parts of the message that match the parameters, and swap them with a link.

It may also be a good first step towards Fluent, as we'll already have the platform side ready.

I'll try to come up with something tomorrow and we can discuss how it looks.

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Okay, I thought about this a bunch yesterday and today, and there's no solution I'm happy with, so at least I'll go with the easier one: I'll use the StringRep to render all error messages, adding a urlCropLimit prop to it. Doing that, we'll have the URL linkification for all error messages, and we can apply the cropping from the client, without losing the actual URL.

I'm not happy with it because it means we're going to parse those error messages from the client, although we know on the platform we're printing URLs (and that I spent a good amount of time on the original patch :/).
But I think that's the wiser move here, given the mismatching timelines of warning message grouping and Fluent.
Also, this will be easy to revert/adjust once we think we're ready to have message metadata sent from platform to client.

Comment 24

[Axel Hecht [:Pike]]

sgtm, thanks.

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1493361 - Add a `urlCropLimit` prop in StringRep. r=Honza.

This new prop allow us to define a maximum length for indivual
URLs (as opposed to cropLimit, which sets it for the whole text).
The URL regex is also modified to be able to linkify messages
wrapped in curly quotes.
Some tests are added to ensure we handle this prop as expected.

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1493361 - Use StringRep to render PageError message. r=Honza.

This allows us to benefit from the linkification that
is done there. We crop the URL at 120 chars for now. We might
consider bumping this up depending on the feedback we get.

We add both a mocha and a mochitest to make sure this work as expected.

Depends on D29006

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/395e27a912b3
Add a `urlCropLimit` prop in StringRep. r=Honza.
https://hg.mozilla.org/integration/autoland/rev/b87cdb455083
Use StringRep to render PageError message. r=Honza.

Comment 28

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/395e27a912b3
https://hg.mozilla.org/mozilla-central/rev/b87cdb455083