Closed Bug 1269426 Opened 4 years ago Closed 4 years ago

browser_bug906190.js in e10s mode would have memory leak

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s + ---
firefox49 --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 8 obsolete files)

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
Assignee: nobody → hchang
Attached file memory leak logs (obsolete) —
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
Whiteboard: btpp-active
Attached file docshell-tree.txt (obsolete) —
Attached file docshell-tree.txt (obsolete) —
Attachment #8748461 - Attachment is obsolete: true
Attached file httpchannelchild-tree.txt (obsolete) —
Attached file httpchannelchild-tree.txt (obsolete) —
Attachment #8748563 - Attachment is obsolete: true
Attached file httpchannelchild-tree.txt (obsolete) —
Attachment #8748567 - Attachment is obsolete: true
Attached patch Bug1269426.patch (obsolete) — Splinter Review
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
Flags: needinfo?(tanvi)
Flags: needinfo?(ckerschb)
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).
Flags: needinfo?(ckerschb)
Attachment #8748976 - Flags: review?(ckerschb)
According to comment 11, Henry's patch could be a potential solution to bug 914860.
See Also: → 914860
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!
Attachment #8748976 - Flags: review?(ckerschb)
Attachment #8748976 - Flags: review?(bugs)
Attachment #8748976 - Flags: review+
(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 :)
Flags: needinfo?(tanvi)
Duplicate of this bug: 914860
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.
Attachment #8748976 - Flags: review?(bugs) → review+
Attachment #8747808 - Attachment is obsolete: true
Attachment #8748513 - Attachment is obsolete: true
Attachment #8748568 - Attachment is obsolete: true
Attachment #8748586 - Attachment is obsolete: true
Attachment #8748976 - Attachment is obsolete: true
(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!
Flags: needinfo?(ckerschb)
Flags: needinfo?(bugs)
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!
Attachment #8752151 - Flags: review+
Clearing needinfo!
Flags: needinfo?(ckerschb)
looks ok
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/6989c81dee4d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Thanks, Henry!  Good job! :)
Depends on: 1273368
You need to log in before you can comment on or make changes to this bug.