Last Comment Bug 859818 - ConsoleAPIStorage.jsm tries to delete var-declared _consoleStorage
: ConsoleAPIStorage.jsm tries to delete var-declared _consoleStorage
Status: RESOLVED FIXED
[good first bug][lang=js][mentor=msucan]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: Firefox 23
Assigned To: Caio Lima(:caiolima)
:
Mentors:
Depends on:
Blocks: consolecleanup
  Show dependency treegraph
 
Reported: 2013-04-09 07:47 PDT by Colby Russell :crussell (no longer Mozilla-ing)
Modified: 2013-05-14 16:50 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First proposal to solve the bug (4.10 KB, patch)
2013-04-19 17:19 PDT, Caio Lima(:caiolima)
no flags Details | Diff | Splinter Review
The line "delete _consoleStorage" removed (1.07 KB, patch)
2013-04-20 10:02 PDT, Caio Lima(:caiolima)
no flags Details | Diff | Splinter Review
Remove of the /**@private**/ comment and the line "delete _consoleStorage" (1.27 KB, patch)
2013-04-22 05:16 PDT, Caio Lima(:caiolima)
no flags Details | Diff | Splinter Review
Remove of the /**@private**/ comment and the line "delete _consoleStorage" Fixing the patch (1.25 KB, patch)
2013-04-23 12:19 PDT, Caio Lima(:caiolima)
mihai.sucan: review+
Details | Diff | Splinter Review

Description Colby Russell :crussell (no longer Mozilla-ing) 2013-04-09 07:47:45 PDT
ConsoleAPIStorage.jsm declares a JSM-local _consoleStorage object <http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPIStorage.jsm#16>:

> var _consoleStorage = {};

It tries to delete it later <http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPIStorage.jsm#50>:

> /** @private */
> observe: function CS_observe(aSubject, aTopic, aData)
> {
>   if (aTopic == "xpcom-shutdown") {
>     Services.obs.removeObserver(this, "xpcom-shutdown");
>     Services.obs.removeObserver(this, "inner-window-destroyed");
>     Services.obs.removeObserver(this, "memory-pressure");
>     delete _consoleStorage;
>   }

Since _consoleStorage is declared with var, this won't actually do anything (except throw a SyntaxError if this were strict mode).

Possible choices:
- Make _consoleStorage a property of ConsoleAPIStorage, then do |delete this._consoleStorage| here.
- Just set _consoleStorage to null, and the object can get GCed.

I'm actually not sure what the point of the statement is anyway, so there's another alternative, which is to just remove it altogether.

(Side note: Why is observe marked "@private"?  It's very definitely not private since the whole point of it is for the observer service to call it...)
Comment 1 Caio Lima(:caiolima) 2013-04-19 16:25:51 PDT
crussel, I'm interested in solve this bug, so...Your proposal is to 
- Make _consoleStorage a property of ConsoleAPIStorage, then do |delete this._consoleStorage| here.
- Just set _consoleStorage to null, and the object can get GCed.

If this variable is global, It could being used in anotehr aprt of code...So I'll try to 
-Make _consoleStorage a property of ConsoleAPIStorage, then do |delete this._consoleStorage| here. 
and how could I test if it crashed another part of code?

For example, in the same file 


getEvents: function CS_getEvents(aId)
  {
    if (aId != null) {
      return (_consoleStorage[aId] || []).slice(0);
    }

    let ids = [];

    for each (let events in _consoleStorage) {
      ids.push(events);
    }
<http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPIStorage.jsm#86>

recordEvent: function CS_recordEvent(aWindowID, aEvent)
  {
    let ID = parseInt(aWindowID);
    if (isNaN(ID)) {
      throw new Error("Invalid window ID argument");
    }

    if (!_consoleStorage[ID]) {
      _consoleStorage[ID] = [];
    }
    let storage = _consoleStorage[ID];
    storage.push(aEvent);
<http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPIStorage.jsm#110>

clearEvents: function CS_clearEvents(aId)
  {
    if (aId != null) {
      delete _consoleStorage[aId];
    }
    else {
      _consoleStorage = {};
      Services.obs.notifyObservers(null, "console-storage-reset", null);
    }
<http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPIStorage.jsm#139>

These parts of code need to be changed to.

What do you think? I could put them as this._consoleStorage too.
Comment 2 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-19 16:46:36 PDT
(In reply to Caio Lima(:caiolima) from comment #1)
> If this variable is global, It could being used in anotehr aprt of code...

It *could* be, but it's not. <http://mxr.mozilla.org/mozilla-central/search?string=_consoleStorage>
Comment 3 Caio Lima(:caiolima) 2013-04-19 17:19:09 PDT
Created attachment 739894 [details] [diff] [review]
First proposal to solve the bug

Turn the _consoleStorage variable a property from ConsoleAPIStorage.
Comment 4 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-19 18:05:38 PDT
Caio: You'll want to mark that attachment as review?mihai.sucan@gmail.com
Comment 5 Caio Lima(:caiolima) 2013-04-19 18:13:40 PDT
I think that It needs to be tested. So, I'll want to mark it as review, but I don't know how to do this.
Comment 6 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-19 19:20:10 PDT
Click the [details] link for the attachment, change the "review" field to ?, and put Mihai as the requestee.
Comment 7 Caio Lima(:caiolima) 2013-04-19 20:04:26 PDT
Thank you
Comment 8 Caio Lima(:caiolima) 2013-04-19 20:04:52 PDT
Thank you crussell!
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-19 20:12:25 PDT
I don't think there's any really very good reason to explicitly clear the storage on shutdown (I see no mention of reasoning in bug 612658), so I suggest just removing that line and otherwise leaving things as-is.
Comment 10 Caio Lima(:caiolima) 2013-04-19 20:25:08 PDT
Gavin, 

In my understand, this property is deleted to clear the array from the memory when the xpcom-shutdown is perfomed, avoiding memory trash.

I don't know if it's really necessary, but changing it to a property should avoid conflicts between another global variable too.
Comment 11 Colby Russell :crussell (no longer Mozilla-ing) 2013-04-20 01:39:32 PDT
(In reply to Caio Lima(:caiolima) from comment #10)
> In my understand, this property is deleted to clear the array from the
> memory

That's not what happens though.  GC has to check the whole graph to make sure there are no other properties pointing to the object, so nothing is freed until the point where GC would have otherwise run.  This is why just setting it to null has the same effect on memory use.
Comment 12 Mihai Sucan [:msucan] 2013-04-20 08:46:14 PDT
Comment on attachment 739894 [details] [diff] [review]
First proposal to solve the bug

Thank you for the patch!

Please keep the patch minimal: remove the |delete _consoleStorage| line. Also please remove the /** @private */ comment from CS_observe(). Thanks!
Comment 13 Caio Lima(:caiolima) 2013-04-20 09:33:09 PDT
keep the _consoleStorage as a global var then? I'm with this doubt.
Comment 14 Mihai Sucan [:msucan] 2013-04-20 09:41:41 PDT
(In reply to Caio Lima(:caiolima) from comment #13)
> keep the _consoleStorage as a global var then? I'm with this doubt.

Yes.
Comment 15 Caio Lima(:caiolima) 2013-04-20 10:02:20 PDT
Created attachment 739988 [details] [diff] [review]
The line "delete _consoleStorage" removed
Comment 16 Caio Lima(:caiolima) 2013-04-20 10:06:02 PDT
Sorry, I'm with a problem to sent the correct patch. I would like to restore my source tree to the current mozilla tree without my patch and create a new one...how could I do it?
Comment 17 Caio Lima(:caiolima) 2013-04-20 11:12:49 PDT
I think that Attachment #739988 [details] [diff] is the correct diff. I'll wait for revision then.
Comment 18 Caio Lima(:caiolima) 2013-04-22 05:16:25 PDT
Created attachment 740246 [details] [diff] [review]
Remove of the /**@private**/ comment and the line "delete _consoleStorage"
Comment 19 Mihai Sucan [:msucan] 2013-04-22 08:49:20 PDT
Comment on attachment 740246 [details] [diff] [review]
Remove of the /**@private**/ comment and the line "delete _consoleStorage"

Patch looks good but it fails to apply for me. Can you please fix the patch? Thanks!
Comment 20 Caio Lima(:caiolima) 2013-04-23 12:19:14 PDT
Created attachment 740963 [details] [diff] [review]
Remove of the /**@private**/ comment and the line "delete _consoleStorage" Fixing the patch
Comment 21 Mihai Sucan [:msucan] 2013-04-24 05:38:32 PDT
Comment on attachment 740963 [details] [diff] [review]
Remove of the /**@private**/ comment and the line "delete _consoleStorage" Fixing the patch

Thanks for the updated patch.
Comment 22 Panos Astithas [:past] 2013-04-25 00:56:11 PDT
https://hg.mozilla.org/integration/fx-team/rev/52ecd5996151
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-04-25 10:50:54 PDT
https://hg.mozilla.org/mozilla-central/rev/52ecd5996151

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