Closed Bug 859818 Opened 7 years ago Closed 7 years ago

ConsoleAPIStorage.jsm tries to delete var-declared _consoleStorage

Categories

(DevTools :: Console, defect, P4)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: crussell, Assigned: caiolima)

References

Details

(Whiteboard: [good first bug][lang=js][mentor=msucan])

Attachments

(1 file, 3 obsolete files)

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...)
Whiteboard: [good first bug][lang=js][mentor=msucan]
Priority: -- → P4
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.
(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>
Attached patch First proposal to solve the bug (obsolete) — Splinter Review
Turn the _consoleStorage variable a property from ConsoleAPIStorage.
Attachment #739894 - Flags: review-
Caio: You'll want to mark that attachment as review?mihai.sucan@gmail.com
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.
Click the [details] link for the attachment, change the "review" field to ?, and put Mihai as the requestee.
Attachment #739894 - Flags: review- → review?(mihai.sucan)
Thank you
Thank you crussell!
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.
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.
(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 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!
Attachment #739894 - Flags: review?(mihai.sucan)
keep the _consoleStorage as a global var then? I'm with this doubt.
(In reply to Caio Lima(:caiolima) from comment #13)
> keep the _consoleStorage as a global var then? I'm with this doubt.

Yes.
Attachment #739894 - Attachment is obsolete: true
Attachment #739988 - Flags: review?(mihai.sucan)
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?
I think that Attachment #739988 [details] [diff] is the correct diff. I'll wait for revision then.
Attachment #739988 - Attachment is obsolete: true
Attachment #739988 - Flags: review?(mihai.sucan)
Attachment #740246 - Flags: review?(mihai.sucan)
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!
Attachment #740246 - Flags: review?(mihai.sucan)
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.
Attachment #740963 - Flags: review?(mihai.sucan) → review+
Whiteboard: [good first bug][lang=js][mentor=msucan] → [land-in-fx-team][good first bug][lang=js][mentor=msucan]
Assignee: nobody → ticaiolima
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/52ecd5996151
Whiteboard: [land-in-fx-team][good first bug][lang=js][mentor=msucan] → [fixed-in-fx-team][good first bug][lang=js][mentor=msucan]
https://hg.mozilla.org/mozilla-central/rev/52ecd5996151
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][good first bug][lang=js][mentor=msucan] → [good first bug][lang=js][mentor=msucan]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.