The default bug view has changed. See this FAQ.

Truncate URLs logged in --DOMWINDOW

RESOLVED FIXED in Firefox 17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years 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)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
M4 is regularly hitting the size limit, is my point here...
I'd totally review a patch to truncate this to 1kb or whatever.
(Assignee)

Updated

5 years ago
Blocks: 765224
(Assignee)

Updated

5 years ago
Blocks: 796328
(Assignee)

Comment 3

5 years ago
(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 ++.
(Assignee)

Comment 4

5 years ago
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)

Updated

5 years ago
Assignee: nobody → continuation
(Assignee)

Updated

5 years ago
Summary: Consider truncating URLs logged in ++DOMWINDOW and --DOMWINDOW → Truncate URLs logged in ++DOMWINDOW and --DOMWINDOW
(Assignee)

Updated

5 years ago
Summary: Truncate URLs logged in ++DOMWINDOW and --DOMWINDOW → Truncate URLs logged in --DOMWINDOW
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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
(Assignee)

Comment 9

5 years ago
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)
(Assignee)

Comment 11

5 years ago
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.
Attachment #668254 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 13

5 years ago
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
Last Resolved: 5 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?
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
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+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/67a47e2225e6
status-firefox17: --- → fixed
status-firefox18: --- → fixed
You need to log in before you can comment on or make changes to this bug.