797649 - Truncate URLs logged in --DOMWINDOW

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

3.3MB of the M4 tinderbox log is two lines (!) containing multi-megabyte data URLS like so:

--DOMWINDOW == 23 (0x152e948e0) [serial = 3310] [outer = 0x1508af9c0] [url = data:text/html,%3C%21DOCTYPE% (etc, etc)

--DOMWINDOW == 22 (0x1508afa40) [serial = 3308] [outer = 0x0] [url = data:text/html,%3C%21DOCTYPE%20HTML% (etc, etc)

Perhaps we could consider truncating URLs that we print with the ++DOMWINDOW and --DOMWINDOW things?  These two don't seem to have a corresponding ++DOMWINDOW, so I'm not sure how useful they even are. Does this sound reasonable?

This sounds silly, but 3.3MB is almost 10% of the total size of the log file, which is about 49MB, and when it hits 50MB the log gets truncated and it makes the sherriffs sad.

Comment 1

[Andrew McCreight [:mccr8]]

M4 is regularly hitting the size limit, is my point here...

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'd totally review a patch to truncate this to 1kb or whatever.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> I'd totally review a patch to truncate this to 1kb or whatever.

Great! Does it make sense to log data URL windows? It seems weird to me that there's a -- but no ++.

Comment 4

[Andrew McCreight [:mccr8]]

Come to think of it, I think we don't use ++DOMWINDOW/--DOMWINDOW any more on tinderbox, so maybe it would also make sense to somehow not print it out there at all.  I find it useful locally, but maybe it isn't useful to have in logs?

That would save about 4.5MB in M4, which is about 50% more than just truncation.

Comment 5

[Phil Ringnalda (:philor)]

We do actually use it in tbpl's "analyze this leak" links, though there's only a couple of leaks currently where that's the clue, that a particular test did a ++ or two without a corresponding --.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: truncate to 1000

I tried with a few small values as well as the current large value, and it seemed to work.

Comment 7

[Andrew McCreight [:mccr8]]

Hopefully one platform is enough to Try for this kind of change...
https://tbpl.mozilla.org/?tree=Try&rev=60c9ff63542e

Comment 8

[Ed Morley [:emorley]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> Hopefully one platform is enough to Try for this kind of change...
> https://tbpl.mozilla.org/?tree=Try&rev=60c9ff63542e

../../../dom/base/nsGlobalWindow.cpp:840:26: error: comparison between signed and unsigned integer expressions

Comment 9

[Andrew McCreight [:mccr8]]

Oops, I guess I need to figure out how to locally set ERRRORS_AS_WARNING.
https://tbpl.mozilla.org/?tree=Try&rev=8decbe8231da

Comment 10

[Ed Morley [:emorley]]

Gotta love em... :-)

(ac_add_options --enable-warnings-as-errors)

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: truncate to 1000, fix unsigned vs signed error

Linux64 looks good, which is hopefully sufficient. I'm going to ignore the red in the opt build because this patch didn't change anything outside of DEBUG.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 668254 [details] [diff] [review]
truncate to 1000, fix unsigned vs signed error

r=me

Comment 13

[Andrew McCreight [:mccr8]]

Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3244bbf3e809

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3244bbf3e809

Comment 15

[Ed Morley [:emorley]]

Comment on attachment 668254 [details] [diff] [review]
truncate to 1000, fix unsigned vs signed error

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Excessive log spew, causing issues when viewing/parsing logs on TBPL.
User impact if declined: Sheriff/dev impact only
Testing completed (on m-c, etc.): m-c, aurora
Risk to taking this patch (and alternatives if risky): None, debug builds only & just reduces log output.
String or UUID changes made by this patch: None

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 668254 [details] [diff] [review]
truncate to 1000, fix unsigned vs signed error

[Approval Request Comment]
Bug caused by (feature/regressing bug #): various
User impact if declined: none
Testing completed (on m-c, etc.): It has been on m-c for a week.
Risk to taking this patch (and alternatives if risky): Extremely low, this only affects logging in debug builds.
String or UUID changes made by this patch: 

The purpose of this patch is to make the lives of sheriffs a little easier by making TBPL logs smaller, around 10% smaller in at least one case.  I wouldn't bother with this except that we're going to be stuck with ESR17 for over a year, so I feel like the cumulative benefit over that period to the sheriffs is worth the tiny risk of a debug-only change landing on beta.

Comment 17

[Andrew McCreight [:mccr8]]

I confirmed locally that this will reduce the size of M4 by about 3MB (out of 34MB) even on beta.

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 668254 [details] [diff] [review]
truncate to 1000, fix unsigned vs signed error

extremely low risk, sheriffs++

Comment 19

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-beta/rev/67a47e2225e6