Closed Bug 1336811 Opened 7 years ago Closed 7 years ago

Accessing XMLHttpRequest.responseText leaks the content window

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- wontfix
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: bugzilla-mozilla, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, Whiteboard: [MemShrink][affects AdblockPlus])

Attachments

(3 files)

Attached file Testcase
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

Steps to reproduce:
0. Clean profile
1. Open the Testcase in a new Tab
2. Navigate to about:memory in that Tab
3. Minimize memory usage
4. Measure

│   └──0.06 MB (00.29%) ++ top(none)/detached/window

Notes:
Happens in both e10s and single process (for e10s ensure that the contentproc stays open).

Generating verbose CC/GC logs seems to free the window

2016-09-12-03-04-21-mozilla-central/	ok
2016-09-13-03-04-25-mozilla-central/	broken
https://hg.mozilla.org/mozilla-central/log/?rev=cfdb7af3af2e:f5d043ce6d36&revcount=1000

Maybe:
https://hg.mozilla.org/mozilla-central/rev/0e3040aed6e9
   Bug 1249739 - Improve performance in XHR in workers - part 1 - XHR.responseText must be cached, r=smaug
https://hg.mozilla.org/mozilla-central/rev/d95f98dcd8a4
   Bug 1249739 - Improve performance in XHR in workers - part 4 - Correct cleaning cache for XHR.responseText, r=me
(In reply to Dorando from comment #0)
> Generating verbose CC/GC logs seems to free the window

Does this suggest a CC optimization bug?
Whiteboard: [MemShrink]
Flags: needinfo?(continuation)
Sorry for the delay in getting to this. Thanks for the clear steps to reproduce and the regression range! I wish I'd noticed the regression range you posted before I started trying to figure out the leak. ;)

Anyways, I ran mozregression myself and confirmed that bug 1249739 is the culprit.

For me, verbose logs didn't free the window, but I was seeing something that sounded similar. In the shutdown logs, I see that the window is leaking because the window reflector is marked black, but in the GC log, there's no clear reason why the reflector should be marked black.

I'm not really sure what is going wrong here. I had a theory that maybe the leak was happening because ClearCachedResponseTextValue marks the wrapper black (even though it doesn't need to, because it is never actually returned anywhere) but changing it to GetWrapperPreserveColor() didn't seem to help.
Blocks: 1249739
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(continuation)
Andrea, could you investigate this a little? I'm not sure what could be going wrong, as your patches didn't add any strong references. As I said, I suspect we may be calling ExposeGCThingToActiveJS somehow (maybe via a call to GetWrapper()).
Flags: needinfo?(amarchesini)
If you can't figure anything out by some investigation, I can try using the GDB stuff sfink added in bug 1337072 that lets you add watchpoints on individual mark bits. That would let me figure out what exactly is setting the mark bit on the XHR wrapper, assuming that's what is going wrong.
So one obvious impact of having a [Cached] attribute is that when we store a value in there we preserve the wrapper.  I expect that's the major driving factor here.

An easy way to check whether that's the issue here is to remove http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/dom/bindings/Codegen.py#7743 and see whether that fixes the leak.

I tried to reproduce this and just test whether commenting out all the .responseText gets and instead adding "request.foo = bar" (to trigger wrapper preservation) is enough to cause the leak, but I can't reproduce this reliably enough to make any sense of it.  :(
As you guessed, commenting out the wrapper preservation fixes the leak, and changing the .responseText to |request.foo = request;| still leaks. I still have no idea why the wrappers are getting marked black.
Another weird thing is that I don't see a window reflector (though maybe I'm just missing it?). The window is being held alive by the document, which is being held alive by the document reflector, which is marked black. The only things rooting the document reflector at all are the document reflector and the XHR reflector, both of which are only gray roots, so how is it being marked black?
Assignee: nobody → continuation
Flags: needinfo?(amarchesini)
Depends on: 1340597
I used sfink's patches in bug 1337072 to watch when the grey bits were being cleared on the XHR reflector. First, I found that it was being cleared due to dumping the heap to disk (bug 1340597) which I worked around by backing out the commit mentioned in that bug.

Secondly, I found that this is indeed due to a CC optimization. The XHR is being set to black with this MarkWrapperLive():

  if (hasLiveWrapper || tmp->IsCertainlyAliveForCC()) {
    ...
    if (!hasLiveWrapper && tmp->PreservingWrapper()) {
      tmp->MarkWrapperLive();
    }

IsCertainlyAliveForCC() must be true here, and it is based on the value of mWaitingForOnStopRequest. Presumably the issue is that we're never going to get a reply from about:blank? Is there something that is supposed to kill off networking requests like this when we close the tab? AFAICT this code has not substantively changed since it first landed in bug 725804.
Flags: needinfo?(bugs)
If necko is behaving sanely, it should kill network connections and cut the edges it has to XHR.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #10)
> If necko is behaving sanely, it should kill network connections and cut the
> edges it has to XHR.

I'm not very familiar with this code, but it looks to me like XMLHttpRequestMainThread::CloseRequest() should set mWaitingForOnStopRequest to false. That fixes the leak for me, in both the original and new test cases.
I still need to wait for a try run, but it looks fairly straightforward to me.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b768bec446f2ef6a8903c62878e77add91deadde
Comment on attachment 8838676 [details]
Bug 1336811 - Clear mWaitingForOnStopRequest in CloseRequest.

https://reviewboard.mozilla.org/r/113502/#review115084

Thanks, and at least this should be very safe, since it is just affecting to the optimization.

::: dom/base/crashtests/1336811.html:12
(Diff revision 1)
> +  request.foo = request;
> + }
> + request.foo = request;
> + request.send();
> + request.foo = request;
> + //only one request.responseText needed

what does this comment mean?
Attachment #8838676 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #14)
> what does this comment mean?

I don't know, it is just from the original test case in comment 0. :)

Unfortunately, the test doesn't leak when set as a crash test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0b74c692044462a517e5ac7cc84071e49e6ca1c

I'm not sure why that is.
Oh right, the test doesn't work because we actually do end up tearing down the XHR in shutdown (maybe some other code runs during shutdown that kills off the channel). That's kind of scary. I'll try writing a browser-chrome test for this.
Bug 1326136 seems similar, but the patch there doesn't seem to actually fix this leak.
See Also: → 1326136
Updated with a browser-chrome test that fails without the patch.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8bc09da971bf
Clear mWaitingForOnStopRequest in CloseRequest. r=smaug
Blocks: GhostWindows
https://hg.mozilla.org/mozilla-central/rev/8bc09da971bf
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
I'll nominate this for backporting in a few days, once I can check some telemetry to make sure it doesn't have a big negative impact on CC pause times (and hopefully, that it reduces the number of ghost windows).
Flags: needinfo?(continuation)
Comment on attachment 8838676 [details]
Bug 1336811 - Clear mWaitingForOnStopRequest in CloseRequest.

Approval Request Comment
[Feature/Bug causing the regression]: old, I think, though something more recent seems to have caused us to get a few reports on it.
[User impact if declined]: bad leaks on certain pages, also using AdblockPlus under certain circumstances
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it is a simple patch. It could minor perf regressions, but those are way less bad than the leaks this patch fixes.
[String changes made/needed]: none
Flags: needinfo?(continuation)
Attachment #8838676 - Flags: approval-mozilla-beta?
Attachment #8838676 - Flags: approval-mozilla-aurora?
Whiteboard: [MemShrink] → [MemShrink][affects AdblockPlus]
Keywords: mlk
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Comment on attachment 8838676 [details]
Bug 1336811 - Clear mWaitingForOnStopRequest in CloseRequest.

fix a memory leak, aurora53+, beta52+
Attachment #8838676 - Flags: approval-mozilla-beta?
Attachment #8838676 - Flags: approval-mozilla-beta+
Attachment #8838676 - Flags: approval-mozilla-aurora?
Attachment #8838676 - Flags: approval-mozilla-aurora+
Reproduce this bug (with steps from bug Description) on an older Nightly Version  53.0a1, Build ID  20170123030211, on Windows 7 x64 bit.

Confirm the fix against latest Nightly 54.0a1, Build ID 20170222030329, on Windows 7 x64 bit.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Blocks: 1326095
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.