Closed Bug 1155634 Opened 9 years ago Closed 9 years ago

Intermittent leakcheck | default process: 696 bytes leaked (ConnectionPool, Mutex, ReentrantMonitor, nsTArray_base, nsThread, ...)

Categories

(Core :: Storage: IndexedDB, defect)

Product:
Core
Component:
Storage: IndexedDB
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: cbook, Assigned: bent.mozilla)

References

(
URL
)

Details

(Keywords: intermittent-failure, memory-leak)

Attachments

(3 files)

Patch, v1
2.30 KB, patch
khuey
: review+
lmandel
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-esr38+
bajaj
: approval-mozilla-b2g37+
Details | Diff | Splinter Review
Branch patch, part 1
2.71 KB, patch
Details | Diff | Splinter Review
Branch patch, part 2
6.30 KB, patch
khuey
: review+
Details | Diff | Splinter Review 
Rev5 MacOSX Yosemite 10.10 mozilla-inbound debug test mochitest-browser-chrome-1

https://treeherder.mozilla.org/logviewer.html#?job_id=8975297&repo=mozilla-inbound

03:00:16 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | default process: 696 bytes leaked (ConnectionPool, Mutex, ReentrantMonitor, nsTArray_base, nsThread, ...)
ConnectionPool sounds like Networking to me.
Component: General → Networking
Flags: needinfo?(mcmanus)
connectionpool is actually indexdb
Component: Networking → DOM: IndexedDB
Flags: needinfo?(mcmanus)
Flags: needinfo?(bent.mozilla)
Bug 1110485 was backed out once before for this, it looks like it came back when it re-landed.
(In reply to Chris Manchester [:chmanchester] from comment #8)
> Bug 1110485 was backed out once before for this, it looks like it came back
> when it re-landed.

That backout was in error.  Cache does not run in the bc1 test chunk at all.
(In reply to Ben Kelly [:bkelly] from comment #58)
> That backout was in error.  Cache does not run in the bc1 test chunk at all.

Retriggers on the push prior to bug 1110485 re-landing confirm the leaks being present before it. Will keep bisecting.
I think I know what's going on here.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(bent.mozilla)
Attached patch Patch, v1 — — Splinter Review 
We were creating the ConnectionPool when we made the first Factory actor, but in PB mode (or if permission is denied some other way) then we never ask the QuotaManager to start up (because we don't need it). But without the QuotaManager we never get the Shutdown notification to clean up ConnectionPool.

So this patch moves ConnectionPool creation closer to where we actually use it and at a point guaranteed to be after QuotaManager has been started.
Attachment #8595543 - Flags: review?(khuey)
(This will probably fix bug 1142678)
Please reland, I don't think this is related.
Flags: needinfo?(bent.mozilla) → needinfo?(wkocher)
(Or needinfo me again if you think it is actually related to this patch after your investigation is concluded)
I backed this out in the same push as a backout of bug 1154426 which was my other guess at what caused the failure (excitingly, it too had a green gij(8) on the try run).

If the backout push goes green, I'll reland this on its own.
Flags: needinfo?(wkocher)
keeping ni on myself as a reminder
Flags: needinfo?(wkocher)
Turns out it was bug 1145680. Relanded.
Flags: needinfo?(wkocher)
https://hg.mozilla.org/mozilla-central/rev/bf525e8a7de7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Given bug 1142678, it sounds like we at least want this on 39. Not sure if it's wanted for 38 as well? If this is going to be uplifted, it'll need to be rebased around bug 1131766.
status-firefox38: --- → ?
status-firefox39: --- → affected
status-firefox-esr31: --- → unaffected
Flags: needinfo?(bent.mozilla)
Comment on attachment 8595543 [details] [diff] [review]
Patch, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 994190
User impact if declined: Shutdown leak of parent process, which is really not a big deal and probably not even noticeable. I think this is only useful for backporting so that automated tests are intermittently orange a bit less.
Testing completed: Caught by automated tests
Risk to taking this patch (and alternatives if risky): Low risk, this just delays creating a thread pool until we actually need it.
String or UUID changes made by this patch: None
Flags: needinfo?(bent.mozilla)
Attachment #8595543 - Flags: approval-mozilla-beta?
Attachment #8595543 - Flags: approval-mozilla-b2g37?
Attachment #8595543 - Flags: approval-mozilla-aurora?
Comment on attachment 8595543 [details] [diff] [review]
Patch, v1

Looks safe for Aurora. Given that there is minimal impact and only 2 Betas left in this cycle, I'll let Sylvestre make the call for Beta. To me, the reason to take this in 38 is so that we fix the intermittent for ESR38. (I don't think that's an incredibly strong reason.)
Attachment #8595543 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8595543 [details] [diff] [review]
Patch, v1

[Triage Comment]
Should be in 38 beta 8
Attachment #8595543 - Flags: approval-mozilla-beta? → approval-mozilla-release+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #95)
> If this is going to be uplifted, it'll need to be rebased around bug 1131766.
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed
Flags: needinfo?(bent.mozilla)
Attachment #8595543 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment on attachment 8595543 [details] [diff] [review]
Patch, v1

This missed Fx38 due to the lack of a rebased patch. Nominating for esr38 hopefully still...
Attachment #8595543 - Flags: approval-mozilla-release+ → approval-mozilla-esr38?
Patch for 38
Flags: needinfo?(bent.mozilla)
Branch needed an additional change (part of bug 1131766)
Attachment #8602958 - Flags: review?(khuey)
Attachment #8601184 - Attachment description: Branch patch → Branch patch, part 1
Comment on attachment 8601184 [details] [diff] [review]
Branch patch, part 1

>+++ b/dom/indexedDB/ActorsParent.cpp
>@@ -6538,16 +6528,26 @@ Database::RecvPBackgroundIDBTransactionC
>   MOZ_ASSERT(!mClosed);
> 
>   if (IsInvalidated()) {
>     // This is an expected race. We don't want the child to die here, just don't
>     // actually do any work.
>     return true;
>   }
> 
>+  if (!gTransactionThreadPool) {
>+    nsRefPtr<TransactionThreadPool> threadPool =
>+      TransactionThreadPool::Create();
>+    if (NS_WARN_IF(!threadPool)) {
>+      return nullptr;
>+    }

This "return nullptr" is clearly wrong here. (mt just brought it up as causing build bustage locally, when building beta)

This function's return-type is bool. (as shown by the "return true" in contextual code, up a few lines)
Flags: needinfo?(bent.mozilla)
Depends on: 1164232
(comment 108 is bug 1164232)
Flags: needinfo?(bent.mozilla)
Attachment #8595543 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Taking this patch is ESR38 to simplify the life of the sheriff.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: