Closed
Bug 874252
Opened 11 years ago
Closed 11 years ago
Leak with expando on a DOMError associated with indexedDB request
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: jruderman, Assigned: baku)
References
Details
(Keywords: memory-leak, regression, testcase, Whiteboard: [MemShrink])
Attachments
(1 file, 4 obsolete files)
2.45 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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
Updated•11 years ago
|
Whiteboard: [MemShrink]
Comment 1•11 years ago
|
||
IDBRequest::mError is not cycle-collected. https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBRequest.cpp?rev=1196b497640b#302
(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 ...
Reporter | ||
Comment 3•11 years ago
|
||
"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.
Assignee | ||
Comment 6•11 years ago
|
||
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+
Yes.
Assignee | ||
Comment 10•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=31be9d99b1e9
Attachment #752137 -
Attachment is obsolete: true
Attachment #752173 -
Flags: review+
Assignee | ||
Comment 11•11 years ago
|
||
I want to see a green result on try before asking to land.
Keywords: checkin-needed
![]() |
||
Comment 12•11 years ago
|
||
This is why we should be CCing all outgoing strong refs, period. In my opinion. And yes, please add the test.
Assignee | ||
Comment 13•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 14•11 years ago
|
||
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+
Assignee | ||
Comment 15•11 years ago
|
||
test in a follow up
Attachment #753756 -
Attachment is obsolete: true
Attachment #754457 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 16•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6e88a93ca0
Keywords: checkin-needed
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ed6e88a93ca0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Reporter | ||
Comment 18•11 years ago
|
||
Did the test land?
Flags: needinfo?(amarchesini)
Flags: in-testsuite?
Assignee | ||
Comment 19•11 years ago
|
||
not yet. it's still in my queue of things to do.
Flags: needinfo?(amarchesini)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•