ConsoleAPIStorage.jsm tries to delete var-declared _consoleStorage

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Developer Tools: Console
P4
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: crussell, Assigned: caiolima)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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

Updated

4 years ago
Blocks: 592463
Whiteboard: [good first bug][lang=js][mentor=msucan]

Updated

4 years ago
Priority: -- → P4
(Assignee)

Comment 1

4 years ago
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>
(Assignee)

Comment 3

4 years ago
Created attachment 739894 [details] [diff] [review]
First proposal to solve the bug

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

Comment 5

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #739894 - Flags: review- → review?(mihai.sucan)
(Assignee)

Comment 7

4 years ago
Thank you
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 10

4 years ago
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)
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Comment 15

4 years ago
Created attachment 739988 [details] [diff] [review]
The line "delete _consoleStorage" removed
Attachment #739894 - Attachment is obsolete: true
Attachment #739988 - Flags: review?(mihai.sucan)
(Assignee)

Comment 16

4 years ago
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?
(Assignee)

Comment 17

4 years ago
I think that Attachment #739988 [details] [diff] is the correct diff. I'll wait for revision then.
(Assignee)

Comment 18

4 years ago
Created attachment 740246 [details] [diff] [review]
Remove of the /**@private**/ comment and the line "delete _consoleStorage"
Attachment #739988 - Attachment is obsolete: true
Attachment #740246 - Flags: review?(mihai.sucan)
Attachment #739988 - 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)
(Assignee)

Comment 20

4 years ago
Created attachment 740963 [details] [diff] [review]
Remove of the /**@private**/ comment and the line "delete _consoleStorage" Fixing the patch
Attachment #740246 - Attachment is obsolete: true
Attachment #740963 - 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+

Updated

4 years ago
Whiteboard: [good first bug][lang=js][mentor=msucan] → [land-in-fx-team][good first bug][lang=js][mentor=msucan]

Updated

4 years ago
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
Last Resolved: 4 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
You need to log in before you can comment on or make changes to this bug.