859818 - ConsoleAPIStorage.jsm tries to delete var-declared _consoleStorage

Status: RESOLVED FIXED

(DevTools :: Console, defect, P4)

Description

[Colby Russell :crussell (no longer Mozilla-ing)]

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)]

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)]

(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)]

Attachment: First proposal to solve the bug

Turn the _consoleStorage variable a property from ConsoleAPIStorage.

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Caio: You'll want to mark that attachment as review?[e-mail]

Comment 5

[Caio Lima(:caiolima)]

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)]

Click the [details] link for the attachment, change the "review" field to ?, and put Mihai as the requestee.

Comment 7

[Caio Lima(:caiolima)]

Thank you

Comment 8

[Caio Lima(:caiolima)]

Thank you crussell!

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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)]

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)]

(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]]

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)]

keep the _consoleStorage as a global var then? I'm with this doubt.

Comment 14

[Mihai Sucan [:msucan]]

(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)]

Attachment: The line "delete _consoleStorage" removed

Comment 16

[Caio Lima(:caiolima)]

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)]

I think that Attachment #739988 [details] [diff] is the correct diff. I'll wait for revision then.

Comment 18

[Caio Lima(:caiolima)]

Attachment: Remove of the /**@private**/ comment and the line "delete _consoleStorage"

Comment 19

[Mihai Sucan [:msucan]]

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)]

Attachment: Remove of the /**@private**/ comment and the line "delete _consoleStorage" Fixing the patch

Comment 21

[Mihai Sucan [:msucan]]

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 (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/52ecd5996151

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52ecd5996151