Closed
Bug 797649
Opened 11 years ago
Closed 11 years ago
Truncate URLs logged in --DOMWINDOW
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
(Whiteboard: [sheriff-want])
Attachments
(1 file, 1 obsolete file)
1.04 KB,
patch
|
bzbarsky
:
review+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
M4 is regularly hitting the size limit, is my point here...
![]() |
||
Comment 2•11 years ago
|
||
I'd totally review a patch to truncate this to 1kb or whatever.
Assignee | ||
Comment 3•11 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•11 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.
Comment 5•11 years ago
|
||
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•11 years ago
|
Assignee: nobody → continuation
Assignee | ||
Updated•11 years ago
|
Summary: Consider truncating URLs logged in ++DOMWINDOW and --DOMWINDOW → Truncate URLs logged in ++DOMWINDOW and --DOMWINDOW
Assignee | ||
Updated•11 years ago
|
Summary: Truncate URLs logged in ++DOMWINDOW and --DOMWINDOW → Truncate URLs logged in --DOMWINDOW
Assignee | ||
Comment 6•11 years ago
|
||
I tried with a few small values as well as the current large value, and it seemed to work.
Assignee | ||
Comment 7•11 years ago
|
||
Hopefully one platform is enough to Try for this kind of change... https://tbpl.mozilla.org/?tree=Try&rev=60c9ff63542e
Comment 8•11 years ago
|
||
(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•11 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
Comment 10•11 years ago
|
||
Gotta love em... :-) (ac_add_options --enable-warnings-as-errors)
Assignee | ||
Comment 11•11 years ago
|
||
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•11 years ago
|
Attachment #668095 -
Attachment is obsolete: true
![]() |
||
Comment 12•11 years ago
|
||
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•11 years ago
|
||
Thanks for the review. https://hg.mozilla.org/integration/mozilla-inbound/rev/3244bbf3e809
Version: unspecified → Trunk
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3244bbf3e809
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 15•11 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 #): 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•11 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•11 years ago
|
||
I confirmed locally that this will reduce the size of M4 by about 3MB (out of 34MB) even on beta.
Comment 18•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/67a47e2225e6
status-firefox17:
--- → fixed
status-firefox18:
--- → fixed
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•