Truncate URLs logged in --DOMWINDOW

RESOLVED FIXED in Firefox 17

Status

()

defect
RESOLVED FIXED
7 years ago
5 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 fixed, firefox18 fixed)

Details

(Whiteboard: [sheriff-want])

Attachments

(1 attachment, 1 obsolete attachment)

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.
M4 is regularly hitting the size limit, is my point here...
I'd totally review a patch to truncate this to 1kb or whatever.
Blocks: logspam
Blocks: 796328
(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 ++.
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.
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 --.
Whiteboard: [sheriff-want]
Assignee: nobody → continuation
Summary: Consider truncating URLs logged in ++DOMWINDOW and --DOMWINDOW → Truncate URLs logged in ++DOMWINDOW and --DOMWINDOW
Summary: Truncate URLs logged in ++DOMWINDOW and --DOMWINDOW → Truncate URLs logged in --DOMWINDOW
Posted patch truncate to 1000 (obsolete) — Splinter Review
I tried with a few small values as well as the current large value, and it seemed to work.
Hopefully one platform is enough to Try for this kind of change...
https://tbpl.mozilla.org/?tree=Try&rev=60c9ff63542e
(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
Oops, I guess I need to figure out how to locally set ERRRORS_AS_WARNING.
https://tbpl.mozilla.org/?tree=Try&rev=8decbe8231da
Gotta love em... :-)

(ac_add_options --enable-warnings-as-errors)
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.
Attachment #668254 - Flags: review?(bzbarsky)
Attachment #668095 - Attachment is obsolete: true
Comment on attachment 668254 [details] [diff] [review]
truncate to 1000, fix unsigned vs signed error

r=me
Attachment #668254 - Flags: review?(bzbarsky) → review+
Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3244bbf3e809
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/3244bbf3e809
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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
Attachment #668254 - Flags: approval-mozilla-beta?
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.
I confirmed locally that this will reduce the size of M4 by about 3MB (out of 34MB) even on beta.
Comment on attachment 668254 [details] [diff] [review]
truncate to 1000, fix unsigned vs signed error

extremely low risk, sheriffs++
Attachment #668254 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.