Closed Bug 874252 Opened 7 years ago Closed 7 years ago

Leak with expando on a DOMError associated with indexedDB request

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jruderman, Assigned: baku)

References

Details

(Keywords: memory-leak, regression, testcase, Whiteboard: [MemShrink])

Attachments

(1 file, 4 obsolete files)

Attached file testcase (obsolete) —
With
  user_pref("dom.indexedDB.enabled", false);

The testcase causes Firefox to leak nsGlobalWindow and nsDocument objects.  (You can see the leaks by running a debug build with XPCOM_MEM_LEAK_LOG=2).

Based on when my DOM fuzzer started reporting this, I'm guessing it's a regression from:

changeset:   http://hg.mozilla.org/mozilla-central/rev/1196b497640b
user:        Andrea Marchesini
date:        Sat May 18 13:52:06 2013 -0400
summary:     Bug 870856 - Convert DOMError to WebIDL. r=Ms2ger, r=bz
Blocks: 870856
Whiteboard: [MemShrink]
(In reply to Masatoshi Kimura [:emk] from comment #1)
> IDBRequest::mError is not cycle-collected.
> https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBRequest.
> cpp?rev=1196b497640b#302

Yeah, that didn't use to matter, but now it does.

Worth checking other DOM objects too ...
"Checking" statically, dynamically, or manually?
Manually would be a good place to start.
e.g. content/media/webspeech/recognition/SpeechRecognition.h appears to have a similar problem.  There are probably more.
Attached patch patch (obsolete) — Splinter Review
I don't see any problem in SpeechRecognition.
Attachment #752137 - Flags: review?(khuey)
Comment on attachment 752137 [details] [diff] [review]
patch

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

Ah I misread that file.  I didn't realize that SpeechEvent is a separate class.
Attachment #752137 - Flags: review?(khuey) → review+
I didn't add the test... should I?
Keywords: checkin-needed
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=31be9d99b1e9
Attachment #752137 - Attachment is obsolete: true
Attachment #752173 - Flags: review+
I want to see a green result on try before asking to land.
Keywords: checkin-needed
This is why we should be CCing all outgoing strong refs, period.  In my opinion.

And yes, please add the test.
Yep, tests are included in the patch.
and the first try request was contains too many unrelated patches.

https://tbpl.mozilla.org/?tree=Try&rev=8747406aefe8
Assignee: nobody → amarchesini
Attached patch patch (obsolete) — Splinter Review
Patch is ready but I still having problem with test_ipc.html. It's not able to catch an exception generated by indexedDB.deleteDatabase().

Test_ipc.html runs all the devicestorage mochitests but it fails with test_bug874252.html. Any hint? Of course test_bug874252.html is green when it runs out of IPC.

The exception is InvalidStateError, and it's generated because in order to test this bug I disabled devicestorage via prefs.
Attachment #751879 - Attachment is obsolete: true
Attachment #752173 - Attachment is obsolete: true
Attachment #753756 - Flags: review+
Attached patch patchSplinter Review
test in a follow up
Attachment #753756 - Attachment is obsolete: true
Attachment #754457 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed6e88a93ca0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Did the test land?
Flags: needinfo?(amarchesini)
Flags: in-testsuite?
not yet. it's still in my queue of things to do.
Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.