1269426 - browser_bug906190.js in e10s mode would have memory leak

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Henry Chang [:hchang]]

Bug 1093642 fixed browser_bug906190.js in e10s mode it would have memory leak after the testing. Specifically the leak is caused by the subtests (5) and (6) [1]. 

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#191

Comment 1

[Henry Chang [:hchang]]

Attachment: memory leak logs

Comment 2

[Henry Chang [:hchang]]

Some findings:

Run sub-test (5) only after removing "gIdentityHandler.disableMixedContentProtection" in [1] and there would be no memory leak. Let's see if I can reproduce the leak by manually doing the same steps.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug906190.js#33

Comment 3

[Henry Chang [:hchang]]

Attachment: docshell-tree.txt

Comment 4

[Henry Chang [:hchang]]

Attachment: docshell-tree.txt

Comment 5

[Henry Chang [:hchang]]

Attachment: httpchannelchild-tree.txt

Comment 6

[Henry Chang [:hchang]]

Attachment: httpchannelchild-tree.txt

Comment 7

[Henry Chang [:hchang]]

Attachment: httpchannelchild-tree.txt

Comment 8

[Henry Chang [:hchang]]

Attachment: Bug1269426.patch

Comment 9

[Henry Chang [:hchang]]

The leak is caused by a cross reference between nsDocShell and its mMixedContentChannel. When nsDocShell::mMixedContentChannel is set to non-null, a refPtr cycle is formed since mMixedContentChannel::mCallbacks is nsDocShell. If nsDocShell::mMixedContentChannel is not redirected, HttpChannelChild::ReleaseListeners() will be called in HttpChannelChild::OnStopRequest() [2] to break the cycle. However, if nsDocShell::mMixedContentChannel is redirected, there's no chance to break the cycle since HttpChannelChild::Redirect3Complete doesn't clear mCallbacks.

There are two solutions that come to my mind:

1) HttpChannelChild to break the cycle: this is what the attached patch does. 
2) nsDocShell to break the cycle: maybe we should clear mMixedContentChannel or reset to aNewChannel.

Both solutions works but (1) seems to be a dramatic change. 

Tanvi, Christopher, what do you think of this? Thanks!


[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10837
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#907
[3] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1388
[4] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7348

Comment 10

[Henry Chang [:hchang]]

Attachment: Bug1269426_I-prefer-this-one.patch

Comment 11

[Henry Chang [:hchang]]

Attached a new patch which should be the best solution since it fixes Bug 914860 as well.

When redirection is happening and the old channel is allowed to load mixed content, we have to either forward the permission to the new channel (if same origin) or clear mMixedContentChannel (if different origin).

Comment 12

[Ethan Tseng [:ethan]]

According to comment 11, Henry's patch could be a potential solution to bug 914860.

Comment 13

[Henry Chang [:hchang]]

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

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8748976 [details] [diff] [review]
Bug1269426_I-prefer-this-one.patch

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

Henry, thanks for fixing this. Patch looks good in my opinion, but we need a docshell peer to sign off on it. Smaug is familiar with mMixedContentChannel; probably he can accept the patch.

::: docshell/base/nsDocShell.cpp
@@ +7353,5 @@
>    NS_ASSERTION(aStateFlags & STATE_REDIRECTING,
>                 "Calling OnRedirectStateChange when there is no redirect");
> +
> +  // If mixed content is allowed for the old channel, we forward
> +  // the permission to the new channel if it has the same origin 

nit: trailing whitespace!

Comment 15

[Tanvi Vyas[:tanvi]]

(In reply to Henry Chang [:henry] from comment #11)
> Attached a new patch which should be the best solution since it fixes Bug
> 914860 as well.
> 
> When redirection is happening and the old channel is allowed to load mixed
> content, we have to either forward the permission to the new channel (if
> same origin) or clear mMixedContentChannel (if different origin).

That's great Henry!  Thanks for debugging this and figuring out a good solution that also fixes another open bug :)

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8748976 [details] [diff] [review]
Bug1269426_I-prefer-this-one.patch

I guess this follows the logic we have in nsDocShell::DoURILoad.
Tiny bit scary still, does this end up breaking the cycle in all the cases.
But fine.

Comment 18

[Henry Chang [:hchang]]

Attachment: Bug1269426.patch (r+'ed by Christopher&Olli)

Comment 19

[Henry Chang [:hchang]]

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8748976 [details] [diff] [review]
> Bug1269426_I-prefer-this-one.patch
> 
> I guess this follows the logic we have in nsDocShell::DoURILoad.
> Tiny bit scary still, does this end up breaking the cycle in all the cases.
> But fine.

Thanks Olli :)

I am not 100% it ends up breaking the cycle in all the cases so I added the 
redirection failure case to see if it would lead to memory leak in the latest
patch.

Olli, Christopher, would you mind reviewing the updated patch again? Thanks!

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8752151 [details] [diff] [review]
Bug1269426.patch (r+'ed by Christopher&Olli)

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

Still fine with me!

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Clearing needinfo!

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

looks ok

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6989c81dee4d

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/6989c81dee4d

Comment 25

[Ethan Tseng [:ethan]]

Thanks, Henry!  Good job! :)