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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Guilherme Lima]

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

Comment 1

[Guilherme Lima]

Sorry to needinfo you, but it seems this went unnoticed and at least a triage would be good from someone you could point out.

Comment 2

[Nicholas Nethercote [inactive]]

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?

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 5

[Thomas Nguyen (:tnguyen)]

hmm, likely nsChannelClassifier did not remove the observer (and fews place in url-classifier), I will looking at that to see what will happen.

Comment 7

[Nathan Froyd [:froydnj]]

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

Comment 8

[Nicholas Nethercote [inactive]]

[Tracking Requested - why for this release]: Memory leak. (Carrying over the tracking request from bug 1339845, which was dup'd to this bug.)

Comment 9

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1336240 - Remove observer in PendingLookup and nsChannelClassifier

Review commit: https://reviewboard.mozilla.org/r/112956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112956/

Comment 10

[Thomas Nguyen (:tnguyen)]

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.

Comment 11

[Marcia Knous [:marcia]]

Tracking 53/54+ for this memory leak.

Comment 12

[Dimi Lee [:dimi]]

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.

Comment 13

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8837968 [details]
Bug 1336240 - Remove observer in PendingLookup and nsChannelClassifier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112956/diff/1-2/

Comment 14

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8837968 [details]
Bug 1336240 - Remove observer in PendingLookup and nsChannelClassifier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112956/diff/2-3/

Comment 15

[François Marier [:francois]]

Comment on attachment 8837968 [details]
Bug 1336240 - Remove observer in PendingLookup and nsChannelClassifier

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

Comment 16

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8837968 [details]
Bug 1336240 - Remove observer in PendingLookup and nsChannelClassifier

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112956/diff/3-4/

Comment 17

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b78f5243f7b4

Comment 18

[Thomas Nguyen (:tnguyen)]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/652362fc7c1b
Remove observer in PendingLookup and nsChannelClassifier r=dimi,francois

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/652362fc7c1b

Comment 21

[Sylvestre Ledru [:Sylvestre]]

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

Comment 22

[Thomas Nguyen (:tnguyen)]

Attachment: Aurora patch uplift

Comment 23

[Thomas Nguyen (:tnguyen)]

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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8841436 [details] [diff] [review]
Aurora patch uplift

Fixing a memory leak sounds good, let's take this for 53 aurora.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34e95ff8e264