Closed Bug 1336240 Opened 3 years ago Closed 3 years ago

Huge number of observer-service-suspect / profile-change-net-teardown

Categories

(Core :: Networking, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 + fixed
firefox54 + fixed

People

(Reporter: guijoselito, Assigned: tnguyen)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files)

It seems this started happening a few days ago, not sure exactly when (first noted on the 20170127 Nightly build).

Checking about:memory, you'll see

17,242 (100.0%) -- observer-service
└──17,242 (100.0%) -- referent
   ├──16,905 (98.05%) ── strong
   └─────337 (01.95%) -- weak
         ├──337 (01.95%) ── alive
         └────0 (00.00%) ── dead

16,269 (100.0%) -- observer-service-suspect
├──15,852 (97.44%) ── referent(topic=profile-change-net-teardown)
├─────232 (01.43%) ── referent(topic=xpcom-shutdown)
└─────185 (01.14%) ── referent(topic=memory-pressure)

It seems the number keeps growing slowly over time.
I'm on Windows and a guy on Linux confirmed to see this as well, so marking the platform as "All".
Sorry to needinfo you, but it seems this went unnoticed and at least a triage would be good from someone you could point out.
Flags: needinfo?(n.nethercote)
So, we have large and growing numbers of "profile-change-net-teardown" observers:

> 16,269 (100.0%) -- observer-service-suspect
> ├──15,852 (97.44%) ── referent(topic=profile-change-net-teardown)
> ├─────232 (01.43%) ── referent(topic=xpcom-shutdown)
> └─────185 (01.14%) ── referent(topic=memory-pressure)

This is suggestive of a leak, e.g. we're registering observers but forgetting to unregister them. Do you know about the "profile-change-net-teardown" event.

jib, you landed bug 1329193 recently which adds "profile-change-net-teardown" observers but doesn't seem to remove them.

tnguyen, you did likewise in bug 1324820.

Could either of those changes be causing this? Should observers be getting removed in either case?
Flags: needinfo?(tnguyen)
Flags: needinfo?(jib)
Whiteboard: [MemShrink]
Flags: needinfo?(n.nethercote)
Sorry bug 1329193 is feature-neutral and does not touch "profile-change-net-teardown" except for unavoidable code indentation: https://reviewboard.mozilla.org/r/102926/diff/3#index_header

Might mozregression find it?
Flags: needinfo?(jib)
That said, you're right that PeerConnection.js never removes any of its observers [1].

AFAIK it's never done that though, so it's not a change.

https://dxr.mozilla.org/mozilla-central/rev/af8a2573d0f1e9cc6f2ba0ab67d7a702a197f177/dom/media/PeerConnection.js#66
hmm, likely nsChannelClassifier did not remove the observer (and fews place in url-classifier), I will looking at that to see what will happen.
Flags: needinfo?(tnguyen)
Duplicate of this bug: 1339845
(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> That said, you're right that PeerConnection.js never removes any of its
> observers [1].

PeerConnection doesn't remove its observers because they're weak observers, so they will naturally go away when the underlying JS object goes away.
[Tracking Requested - why for this release]: Memory leak. (Carrying over the tracking request from bug 1339845, which was dup'd to this bug.)
Scanned download protection and channel classifier, there're 2 observers we need to remove (PendingLookup and nsChannelClassifier would call addobserver multiple times and need to be removed). I prefer calling RemoveObserver than using weak-referrence there.
Tracking 53/54+ for this memory leak.
Attachment #8837968 - Flags: review?(dlee)
Comment on attachment 8837968 [details]
Bug 1336240 - Remove observer in PendingLookup and nsChannelClassifier

https://reviewboard.mozilla.org/r/112956/#review115312

looks good to me, thanks!

::: netwerk/base/nsChannelClassifier.cpp:441
(Diff revision 1)
>  
>      // Add an observer for shutdown
> -    nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> -    if (!observerService)
> -      return NS_ERROR_FAILURE;
> +    rv = AddShutdownObserver();
> +    if (NS_FAILED(rv)) {
> +      LOG(("nsChannelClassifier[%p]: Couldn't register shutdown observer", this));
> +      return rv;

I think we shouldn't do error return here, fail to register an observer for any reason could still be able to do classify.

::: toolkit/components/downloads/ApplicationReputation.cpp:1013
(Diff revision 1)
> +  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +  if (!observerService) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  observerService->AddObserver(this, "quit-application", false);

In the case of PendingLookup we need to remove observer in OnComplete, Notify and Observe I'll suggest we use reference to remove observer here.
Attachment #8837968 - Flags: review?(dlee) → review+
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Attachment #8837968 - Flags: review?(francois)
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment on attachment 8837968 [details]
Bug 1336240 - Remove observer in PendingLookup and nsChannelClassifier

https://reviewboard.mozilla.org/r/112956/#review116504
Attachment #8837968 - Flags: review?(francois) → review+
Comment on attachment 8837968 [details]
Bug 1336240 - Remove observer in PendingLookup and nsChannelClassifier

Approval Request Comment
[Feature/Bug causing the regression]: bug 1324820
[User impact if declined]: Memory leak
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Described in comment 1
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: No
[Why is the change risky/not risky?]: Low, we only remove the observe in the observer list
[String changes made/needed]: No
Attachment #8837968 - Flags: approval-mozilla-aurora?
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/652362fc7c1b
Remove observer in PendingLookup and nsChannelClassifier r=dimi,francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/652362fc7c1b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Doesn't apply to aurora

netwerk/base/nsChannelClassifier.h! (edit, then use 'hg resolve --mark') 
abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(tnguyen)
Flags: needinfo?(tnguyen)
Attachment #8837968 - Flags: approval-mozilla-aurora?
Comment on attachment 8841436 [details] [diff] [review]
Aurora patch uplift

Approval Request Comment
[Feature/Bug causing the regression]: bug 1324820
[User impact if declined]: Memory leak
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Described in comment 1
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: No
[Why is the change risky/not risky?]: Low, we only remove the observe in the observer list
[String changes made/needed]: No
Attachment #8841436 - Flags: approval-mozilla-aurora?
Comment on attachment 8841436 [details] [diff] [review]
Aurora patch uplift

Fixing a memory leak sounds good, let's take this for 53 aurora.
Attachment #8841436 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Untriaged → Networking
Product: Firefox → Core
Target Milestone: Firefox 54 → ---
Blocks: 1324820
Target Milestone: --- → mozilla54
Version: Trunk → 53 Branch
You need to log in before you can comment on or make changes to this bug.