URLs reported to the console can be overly long

RESOLVED FIXED in Firefox 68

Status

enhancement
P1
normal
RESOLVED FIXED
9 months ago
Last month

People

(Reporter: Ehsan, Assigned: nchevobbe)

Tracking

(Blocks 1 bug)

unspecified
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [console-grouping])

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

9 months ago
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!
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.
(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...
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.
Reporter

Comment 5

9 months ago
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.
Flags: needinfo?(bgrinstead)
Comment on attachment 9011172 [details]
Bug 1493361 - Truncate the URLs reported to the console

Andrea Marchesini [:baku] has approved the revision.
Attachment #9011172 - Flags: review+
(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.
Flags: needinfo?(bgrinstead) → needinfo?(nchevobbe)
Reporter

Comment 8

9 months ago
(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?
Reporter

Comment 9

9 months ago
(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.)
Assignee

Comment 10

7 months ago
> 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.
Flags: needinfo?(nchevobbe)
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.
Blocks: 1462372
Severity: normal → enhancement
Component: Security → Console
Priority: -- → P2
Product: Core → DevTools
Target Milestone: --- → Firefox 66
Reporter

Comment 12

6 months ago
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...
Reporter

Updated

6 months ago
Assignee: ehsan → nobody
Status: ASSIGNED → NEW
Attachment #9011172 - Attachment is obsolete: true
Assignee

Updated

5 months ago
Whiteboard: [console-unclutter]
Assignee

Comment 13

5 months ago

I'm going to give this a try

Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [console-unclutter] → [console-grouping]
Assignee

Updated

2 months ago
Blocks: 1545888
Assignee

Comment 17

2 months ago

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!

Flags: needinfo?(ehsan)
Reporter

Comment 18

2 months ago

(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!

Flags: needinfo?(ehsan)
Assignee

Comment 19

2 months ago

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?).

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

Flags: needinfo?(l10n)

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.

Flags: needinfo?(l10n)
Assignee

Comment 22

2 months ago

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.

Assignee

Comment 23

2 months ago

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.

sgtm, thanks.

Assignee

Comment 25

2 months ago

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.

Assignee

Comment 26

2 months ago

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

Attachment #9060382 - Attachment is obsolete: true
Attachment #9060383 - Attachment is obsolete: true
Attachment #9060385 - Attachment is obsolete: true

Comment 27

2 months ago
Pushed by nchevobbe@mozilla.com:
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

Last month
bugherder
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: Firefox 66 → Firefox 68
You need to log in before you can comment on or make changes to this bug.