Closed Bug 885751 Opened 7 years ago Closed 7 years ago

dbg-server.jsm compartment is leaking 500,000-char JavaScript warning strings like a sieve

Categories

(DevTools :: Debugger, defect, P1, major)

21 Branch
x86_64
Linux
defect

Tracking

(firefox24 verified, firefox25 verified)

VERIFIED FIXED
Firefox 25
Tracking Status
firefox24 --- verified
firefox25 --- verified

People

(Reporter: me, Assigned: msucan)

Details

Attachments

(2 files)

My Firefox instance is currently using about 2.5GB of RAM.

1GB of that I consider quite reasonable. (I'm running a load which experience suggests Chrome would be using well over 2GB for, plus almost 200 lazily-not-loaded-at-present tabs which I believe Chrome doesn't support, let alone the Panorama feature which I use to manage most of it to good effect. Even with some problems, I love Firefox; switching to Chrome is unthinkable.)

1.5GB of that appears to be leaked memory from the JavaScript warning logging strings.

Steps used to find the problem:

1. Start a new project using Foundation (the SASS version).
2. Start working on the page, refreshing the page rather regularly as changes are made and most of the time having one or both of Firebug and the Inspector of the Dev Tools open (I opened all the other tabs somewhere along the way, which could potentially matter), but opening and closing both of them fairly frequently (not for particularly good reasons).
3. Wonder why your 4GB of RAM is almost all consumed †.

about:memory shows explicit/js-non-window/compartments/non-window-global/compartment([System Principal], [anonymous sandbox] (from: resource://gre/modules/devtools/dbg-server.jsm:41))/string-chars using 1,529,840,416 B.

All of the strings being displayed in "huge" are between 489977 and 490234 characters long. This is suspiciously close to 500000 and the size spread is 258 characters, suspiciously close to 256.

All the strings of size start with "[JavaScript Warning: ". I don't know what comes after the first 32 bytes, though, because about:memory doesn't say that. :-(

None of the strings are presently available in the Error Console or the Web Console, and I have closed the Developer Tools and Firebug on all tabs (I believe).

In case it helps, the page that I'm working on has contained experimentation with clip-path, hyperlinks, CSS, a few small images, blocks of colour and a good deal of sadness about features that Chrome and Internet Explorer (especially the latter) are lacking or implement badly. (Chrome, why is your clip-path not not clipping the hit target? That makes it useless for me...) Oh, it's also got some inline SVG now, because clip-path couldn't do what I needed in WebKit. No JavaScript beyond the defaults of Foundation. (Modernizr, Zepto and a handful of libraries of their own.)

The page that I'm working on is a local page and I am not at present willing to share it. Sorry about that.

I have not yet tested it on Nightly, nor was I immediately able to reproduce the issue with a believed-similar load on a blank Firefox profile with only Firebug added.

At the time of writing, I still have the same instance of Firefox running, but it's likely to be killed if there's no other way to free that 1.5GB of RAM within a day or so.

Other potentially useful information
====================================

:User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0

:Enabled add-ons:

  - Adblock Plus
  - Atlassian Bonfire
  - Context Font
  - DownThemAll!
  - DuckDuckGo Plus
  - Firebug (1.11.2 )
  - FireFontFamily
  - Greasemonkey
  - PDF Viewer
  - Stylish
  - View Dependencies
  - Plus the standard ones provided for Ubuntu:
    - Global Menu Bar integration
    - Ubuntu Firefox Modifications
    - Ubuntu Online Accounts
    - Unity Desktop Integration
    - Unity Websites integration

† This step is optional. Especially, don't worry about removing 28GB of your RAM if you're lucky enough to have that much surplus.
I suspect this is related to bug 877773. There, Firefox is generating warnings about a particular point in the code. the nsIScriptError instance is trying to hand out the source line on which the warning occurred. Unfortunately, because the code is minimized, that source line is... the entire text. Thus, every error message includes a fresh copy of the entire script text.

If that is the case, we really need to fix nsIScriptError sooner rather than later. It's true that we shouldn't be retaining all those strings, but it's also not helpful to even generate them in the first place.
We're creating LongStringActors with the full line (therefore the whole minified file) with every warning generated.  They're only being cleaned up when the toolbox is closed entirely.

There are a few things we can do to make this better in the long run, but in the short term I think we need to stop allocating LongStringActors here.  We should send the client a long-enough string to be reasonable (I think we discussed 500 chars).

Longer-term, nsIScriptError could be passed a SourceText and offset info instead of a sourceLine.  Then we can get the script text lazily, from the original copy stored by the JS engine.
Will we need to get this on branches?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Sounds good wrt. the proposed short-term solution. I can make a patch for this. Today is rather late for me. I'll do one tomorrow. We should be able to land it in time for aurora. If not, I think this is something we can get approval for aurora anyway.

Should we limit the string to the initial size of text in a LSA, DebuggerServer.LONG_STRING_INITIAL_LENGTH? Anything beyond that gets cut off.
Bug investigation: I cannot reproduce with Firefox Nightlies. I load youtube pages, github pages and google plus pages all at once with the browser console open. dbg-server only uses up to 5mb of memory and it doesn't have any string-chars listed.

Things to note:

1. User agent shown in this bug report that of Firefox 21. This is the stable channel - before bug 877773.
2. The strings mentioned in the report start with "[JavaScript Warning:" which means these are not from the nsIScriptError.sourceLine property. Also their sizes point to the |message| property, which I remember had similar sizes on youtube. We no longer read that property, since bug 877773.
3. We automatically purge LSA's for all messages as they get purged from the web/browser console output. So, as you refresh/load pages, and get more and more errors, they also get removed.

If this bug was reported for Firefox 21, not for nightlies, then maybe bug 877773 fixed it.

I played with the console actor and by changing the code to not use a LSA for sourceLine - just cut the string off after a predefined length - I lowered memory use for dbg-server.jsm to under 1mb with the same pages open (as mentioned above). Will submit a patch.
Attached patch proposed patchSplinter Review
This patch limits the length of the |sourceLine| and it removes the use of LSA for this property.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #766414 - Flags: review?(dcamp)
Comment on attachment 766414 [details] [diff] [review]
proposed patch

Review of attachment 766414 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/webconsole.js
@@ +550,5 @@
>              }
>              else {
>                message = {
>                  _type: "LogMessage",
> +                message: this._createStringGrip(aMessage.message),

Can you explain this change please?
(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 766414 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 766414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/webconsole.js
> @@ +550,5 @@
> >              }
> >              else {
> >                message = {
> >                  _type: "LogMessage",
> > +                message: this._createStringGrip(aMessage.message),
> 
> Can you explain this change please?

It seems that was forgotten in bug 877773.
Attachment #766414 - Flags: review?(dcamp) → review+
https://hg.mozilla.org/mozilla-central/rev/743be08f2381
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Comment on attachment 766414 [details] [diff] [review]
proposed patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug report is not clear enough. We may have increased memory usage with the patch for bug 877773 - we started to use LongStringActors for long strings, including for script error source lines. This means such long lines are kept in the debugger server compartment until the devtools are closed, or until the web console output is cleared.
User impact if declined: minimal to moderate. The memory usage is not expected to increase too much in general cases, see comment 5 for details.
Testing completed (on m-c, etc.): landed in fx-team and m-c.
Risk to taking this patch (and alternatives if risky): minimal.
String or IDL/UUID changes made by this patch: none.

Thank you!
Attachment #766414 - Flags: approval-mozilla-aurora?
Comment on attachment 766414 [details] [diff] [review]
proposed patch

Approving for aurora as this may be a fallout form 877773 and has memory wins
Attachment #766414 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm certainly happy with the description of the fix; I know that the initial report isn't very clear, but I couldn't really make it clearer, and my initial attempts to reproduce it failed. The fix certainly should fix, at the very least, the severity of the problem, making it use only 1MB of memory rather than 1GB in my particular case. That's small enough that I'm not quite so much bothered if there is a leak, though naturally any leaks are undesirable.

Remember with regards to bug 877773 that I was running Firefox 21, while that bug is targeting Firefox 24.

Thanks for working on this!
Chris, can you please check the latest Beta and Aurora to confirm this is fixed?
Flags: needinfo?(me)
I haven't observed the problem again, so it may as well be considered to be fixed.
Flags: needinfo?(me)
Thanks Chris, calling it verified.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.