Last Comment Bug 797649 - Truncate URLs logged in --DOMWINDOW
: Truncate URLs logged in --DOMWINDOW
Status: RESOLVED FIXED
[sheriff-want]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Andrew McCreight [:mccr8]
:
:
Mentors:
Depends on:
Blocks: logspam 796328
  Show dependency treegraph
 
Reported: 2012-10-03 17:39 PDT by Andrew McCreight [:mccr8]
Modified: 2012-10-13 06:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
truncate to 1000 (1.04 KB, patch)
2012-10-04 11:33 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
truncate to 1000, fix unsigned vs signed error (1.04 KB, patch)
2012-10-04 17:23 PDT, Andrew McCreight [:mccr8]
bzbarsky: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-10-03 17:39:11 PDT
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] 2012-10-03 17:39:40 PDT
M4 is regularly hitting the size limit, is my point here...
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-10-03 17:44:41 PDT
I'd totally review a patch to truncate this to 1kb or whatever.
Comment 3 Andrew McCreight [:mccr8] 2012-10-03 17:55:34 PDT
(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] 2012-10-03 18:16:06 PDT
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) 2012-10-03 19:35:26 PDT
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] 2012-10-04 11:33:58 PDT
Created attachment 668095 [details] [diff] [review]
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] 2012-10-04 11:37:02 PDT
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] 2012-10-04 12:02:54 PDT
(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] 2012-10-04 14:12:07 PDT
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] 2012-10-04 14:17:06 PDT
Gotta love em... :-)

(ac_add_options --enable-warnings-as-errors)
Comment 11 Andrew McCreight [:mccr8] 2012-10-04 17:23:36 PDT
Created attachment 668254 [details] [diff] [review]
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 [:bz] (still a bit busy) 2012-10-04 17:39:16 PDT
Comment on attachment 668254 [details] [diff] [review]
truncate to 1000, fix unsigned vs signed error

r=me
Comment 13 Andrew McCreight [:mccr8] 2012-10-05 12:17:04 PDT
Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3244bbf3e809
Comment 14 Ed Morley [:emorley] 2012-10-06 12:47:44 PDT
https://hg.mozilla.org/mozilla-central/rev/3244bbf3e809
Comment 15 Ed Morley [:emorley] 2012-10-12 04:36:45 PDT
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] 2012-10-12 04:42:12 PDT
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] 2012-10-12 04:43:32 PDT
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] 2012-10-12 14:34:29 PDT
Comment on attachment 668254 [details] [diff] [review]
truncate to 1000, fix unsigned vs signed error

extremely low risk, sheriffs++
Comment 19 Andrew McCreight [:mccr8] 2012-10-13 06:42:56 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/67a47e2225e6

Note You need to log in before you can comment on or make changes to this bug.