1147843 - mInterceptListener is not always cleaned up properly

Status: RESOLVED INVALID

(Core :: Networking: HTTP, defect, P5)

Description

[Josh Matthews [:jdm]]

In two out of three locations in HttpChannelChild.cpp, mInterceptListener has Cleanup called before nulling out the pointer. We should probably make a method that performs these actions to avoid this happening again, since it can lead to a memory leak (there's a cycle between the listener and the channel).

Comment 1

[Donato Sciarra]

Attachment: bug1147843.patch

Comment 2

[Josh Matthews [:jdm]]

It looks fine to me, but we should scrap the MOZ_LIKELY for two reasons - this is actually an uncommon case, and MOZ_LIKELY really only belongs in extremely hot code where we believe it will make a significant difference. For the next revision, please flag :mayhemer for review, since this is code he owns.

Comment 3

[Donato Sciarra]

Attachment: bug1147843.patch

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8586300 [details] [diff] [review]
bug1147843.patch

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +992,5 @@
> +HttpChannelChild::CleanupInterceptListener()
> +{
> +  if (mInterceptListener)
> +    mInterceptListener->Cleanup();
> +  mInterceptListener = nullptr;

put this to the if () block as well.

@@ +1354,5 @@
>        SendCancel(status);
>      if (mSynthesizedResponsePump) {
>        mSynthesizedResponsePump->Cancel(status);
>      }
> +    CleanupInterceptListener();

so, this is the only place different, we also call Cleanup() on the listener (if there is one)

Comment 5

[Donato Sciarra]

Attachment: bug1147843.patch

This should be a final patch. Could you please also assign me the bug?

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8587851 [details] [diff] [review]
bug1147843.patch

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

I r+'ed this already.  No need to ask again.

Comment 7

[Donato Sciarra]

OK. Didn't know that. what are the next steps now? I do not have rights to check-in...

Comment 8

[Josh Matthews [:jdm]]

We should send it to the tryserver for a successful run, then add the `checkin-needed` keyword to the Keywords field up above. If you apply for permission to access the tryserver (see https://www.mozilla.org/en-US/about/governance/policies/commit/), I'll vouch for you.

Comment 9

[Donato Sciarra]

Done: https://treeherder.mozilla.org/#/jobs?repo=try&revision=139425234282
There are 3 Errors which seem to be a know issue [see bug1145507].

Does it look OK or should have I run more tests?

Comment 10

[Josh Matthews [:jdm]]

For this patch, you should really run the mochitests, as no other test suite will end up exercising the changed code.

Comment 11

[Donato Sciarra]

Houston we've got a problem: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2ebd30b971e

Comment 12

[Donato Sciarra]

Attachment: bug1147843.patch

I did another run with the latest patch on the try server. 
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc76cb20be9

The majority of the errors are intermittent ones. There are 4 leakchecks tab-process failing but I am not able to reproduce them on my machine.
I build in debug by adding:
ac_add_options --enable-debug-symbols

and I am running the tests as follow:
./mach mochitests --e10s /dom/workers/test/serviceworkers


how would you proceed from here?

Comment 13

[Josh Matthews [:jdm]]

Ack. I'm really sorry that I never replied here. I'll figure out what the next step is by tomorrow at the latest!

Comment 14

[Josh Matthews [:jdm]]

Wow. I'm really bad at this, and I apologize again. Here's what I'm going to do - the repeated M-e10s (4) leaks worry me, and I want to ensure we have the best possible test coverage for these changes, so I'm going to do another try push focusing on the plain mochitests and the web-platform-test suites.

Also FYI, --enable-debug-symbols is different than --enable-debug (the latter is the one you want in order to make a debug build).

Comment 15

[Jinank Jain[:jj]]

Is this bug still exists?

Comment 16

[Josh Matthews [:jdm]]

Yes, since https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/netwerk/protocol/http/HttpChannelChild.cpp#1657 still doesn't call cleanup. Want to rebase the patch and see what the tryserver thinks of it?

Comment 17

[Jinank Jain[:jj]]

Attachment: bug1147843.diff

Comment 18

[Jinank Jain[:jj]]

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

Comment 19

[Josh Matthews [:jdm]]

No need to use needinfo as well as set a review flag.

Comment 20

[Josh Matthews [:jdm]]

The xpcshell test netwerk/test/unit_ipc/test_synthesized_response_wrap.js (X/X3) is failing consistently on that try push, and the e10s web-platform-test /service-workers/service-worker/fetch-event.https.html (W-e10s 8) is showing an assertion, and several e10s mochitests are also showing the same assertion as well as leaking memory. There's clearly something wrong with this change :(

Comment 21

[Josh Matthews [:jdm]]

Since the only functional change in this patch is calling mInterceptListener->Cleanup() from HttpChannelChild::Cancel, that is obviously the cause of all of these problems. Since InterceptStreamListener::Cleanup clears the mOwner and mContext fields, this means that any subsequent notifications (eg. OnStopRequest) will not be invoked, which explains why the FetchDriver code is failing the assertion that it received all of its notifications.

I propose we try the following:
* remove the use of CleanupInterceptListener from HttpChannelChild::Cancel
* make InterceptStreamListener::OnStopRequest call Cleanup before returning

Comment 22

[Jinank Jain[:jj]]

Attachment: v2.diff

What about this one?

Comment 23

[Josh Matthews [:jdm]]

Comment on attachment 8777862 [details] [diff] [review]
v2.diff

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ -1673,4 @@
>      if (mSynthesizedResponsePump) {
>        mSynthesizedResponsePump->Cancel(status);
>      }
> -    mInterceptListener = nullptr;

I'm pretty sure we can't just remove this line. A try push should confirm it.

Comment 24

[Jinank Jain[:jj]]

(In reply to Josh Matthews [:jdm] from comment #23)
> Comment on attachment 8777862 [details] [diff] [review]
> v2.diff
> 
> Review of attachment 8777862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ -1673,4 @@
> >      if (mSynthesizedResponsePump) {
> >        mSynthesizedResponsePump->Cancel(status);
> >      }
> > -    mInterceptListener = nullptr;
> 
> I'm pretty sure we can't just remove this line. A try push should confirm it.

Got your point so we can add CleanUp method for InterceptStreamListener too.

Comment 25

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 26

[BugBot [:suhaib / :marco]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 27

[Sneha k]

Hello! Could I take up this bug? Can you please explain how to proceed to fix it? thanks :)

Comment 28

[Josh Matthews [:jdm]]

I am no longer a good mentor for this work since I'm not involved with the project.

Comment 29

[kernp25]

Is this bug still valid?

Because, it seems mInterceptListener has been removed?

https://searchfox.org/mozilla-central/search?q=mInterceptListener&path=&case=false&regexp=false

Comment 30

[Dragana Damjanovic [:dragana]]

Thank you for checking his. The bug is not valid anymor.