Closed
Bug 1336811
Opened 8 years ago
Closed 8 years ago
Accessing XMLHttpRequest.responseText leaks the content window
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla54
People
(Reporter: bugzilla-mozilla, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
(Keywords: memory-leak, Whiteboard: [MemShrink][affects AdblockPlus])
Attachments
(3 files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/x-review-board-request
|
smaug
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details |
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
Comment 1•8 years ago
|
||
(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]
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(continuation)
Assignee | ||
Comment 2•8 years ago
|
||
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
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr45:
--- → unaffected
status-firefox-esr52:
--- → affected
Ever confirmed: true
Flags: needinfo?(continuation)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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.
Comment 5•8 years ago
|
||
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. :(
Assignee | ||
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → continuation
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
If necko is behaving sanely, it should kill network connections and cut the edges it has to XHR.
Flags: needinfo?(bugs)
Assignee | ||
Comment 11•8 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
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.
Assignee | ||
Comment 17•8 years ago
|
||
Bug 1326136 seems similar, but the patch there doesn't seem to actually fix this leak.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
Updated with a browser-chrome test that fails without the patch.
Assignee | ||
Updated•8 years ago
|
Comment 20•8 years ago
|
||
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8bc09da971bf
Clear mWaitingForOnStopRequest in CloseRequest. r=smaug
Assignee | ||
Updated•8 years ago
|
Blocks: GhostWindows
Comment 21•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 22•8 years ago
|
||
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)
Assignee | ||
Comment 24•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink][affects AdblockPlus]
Comment 25•8 years ago
|
||
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 26•8 years ago
|
||
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+
Comment 27•8 years ago
|
||
bugherder uplift |
Comment 28•8 years ago
|
||
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.
Comment 29•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
Comment 30•8 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•