Docshell leak with document.write(document.body.innerHTML) and window.location.reload

VERIFIED FIXED in mozilla2.0b10

Status

()

defect
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: martijn.martijn, Assigned: bjarne)

Tracking

({memory-leak, testcase})

Trunk
mozilla2.0b10
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

11 years ago
Posted file testcase
See testcase, when clicking the button, I get a docshell leak according the nspr leak log:
Leaked docshell at address 796400.
 ... which loaded URI "about:blank".
 ... which loaded URI "wyciwyg://0/file:///C:/Documents%20and%20Settings/mw/Bureaublad/crashzone/ajaxdemolisher_files/Kopie%20van%20crash1_98.htm".
 ... which loaded URI "file:///C:/Documents%20and%20Settings/mw/Bureaublad/crashzone/ajaxdemolisher_files/Kopie%20van%20crash1_98.htm".
Still leaks on trunk

nsTraceRefcntImpl::DumpStatistics: 914 entries
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6560 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of Connection with size 104 bytes each (208 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of MatchAutoCompleteFunction with size 12 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Service with size 40 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 32 instances of Statement with size 60 bytes each (1920 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of imgLoader with size 44 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of mozStorageFunctionGetUnreversedHost with size 12 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsBaseURLParser with size 12 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsCacheRequest with size 32 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsCategoryObserver with size 60 bytes each (120 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDSURIContentListener with size 36 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDefaultURIFixup with size 28 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDeque with size 52 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDocLoader with size 152 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDocShell with size 504 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDocShell::InterfaceRequestorProxy with size 16 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDocumentCharsetInfo with size 24 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDocumentOpenInfo with size 44 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDownloadManager with size 68 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsLayoutHistoryState with size 56 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsLoadGroup with size 8 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 3 instances of nsLocalFile with size 88 bytes each (264 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsNavBookmarks with size 256 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsNavHistory with size 504 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsObserverService with size 48 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsPersistentProperties with size 80 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsPrefBranch with size 80 bytes each (160 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsPrefService with size 40 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsPrincipal with size 76 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsSHEntry with size 176 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsSimpleURI with size 52 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsStandardURL with size 188 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 85 instances of nsStringBuffer with size 8 bytes each (680 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsStringBundle with size 40 bytes each (80 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 36 instances of nsTArray_base with size 4 bytes each (144 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 2 instances of nsThread with size 72 bytes each (144 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsURILoader with size 16 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 10 instances of nsVoidArray with size 4 bytes each (40 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 3 instances of nsWeakReference with size 16 bytes each (48 bytes total)
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsWebBrowserFind with size 52 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsWebNavigationInfo with size 20 bytes
TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsWyciwygChannel with size 104 bytes

Comment 2

9 years ago
I finally hit this leak in my own fuzzing. Nasty, seems likely to affect real sites.
blocking2.0: --- → ?
OS: Windows XP → All
Hardware: x86 → All
I've pinned this down.  Necko is dropping an nsCacheRequest on the floor.  That owns the nsWyciwygChannel which owns a bunch of refs to the docshell and then we leak everything.
Assignee: nobody → khuey
Component: DOM → Networking: Cache
QA Contact: general → networking.cache
Specifically, in the if block at [1] Necko pulls the nsCacheRequest out of the linked list.  All of the control paths out of that block destroy the request except for [2] which is where we end up.

[1] http://hg.mozilla.org/mozilla-central/annotate/4dd9e1b6d029/netwerk/cache/nsCacheService.cpp#l2336
[2] http://hg.mozilla.org/mozilla-central/annotate/4dd9e1b6d029/netwerk/cache/nsCacheService.cpp#l2370
Comment on attachment 502155 [details] [diff] [review]
Don't drop the nsCacheRequest on the floor.

We may not know what to do with the request here, but leaking it is wrong.

My apologies for the short diff, idk how to get mq to use more context.  See the links in the previous comment for context.
Attachment #502155 - Flags: review?(cbiesinger)
Cc:ing people who have been involved in recent wyciwyg channel and cache related changes. Please see Kyle's comments above.
Attachment #502155 - Flags: review?(cbiesinger) → review?(michal.novotny)
> idk how to get mq to use more context.

[default]
unified = 8

in your .hgrc.  Or use bzexport and then put unified=8 in [bzexport].
Comment on attachment 502155 [details] [diff] [review]
Don't drop the nsCacheRequest on the floor.

The listener must be notified before deleting the request.

(In reply to comment #6)
> We may not know what to do with the request here, but leaking it is wrong.

It is really a question what should we do here? If we fail and delete the request then we have inconsistent behavior when the requests and successive and when they are concurrent. In case of successive requests the read-only request validates the entry in nsCacheEntry::RequestAccess().


Another question is what should this JS code do? Shouldn't the window.location.reload() implicitly close the wyciwyg document? And should we really reload the wyciwyg document or should we reload the origin page?
Attachment #502155 - Flags: review?(michal.novotny) → review-
Assignee

Comment 10

9 years ago
So this actually can happen... (nice testcase  :) )

It's a READ-ONLY request to an invalid entry (not doomed) and there are three lines in nsCacheService.cpp starting at 2324 with some thoughts which may be worth considering.

One possible solution is to re-post a nsProcessEventRequest for such requests. I have implemented this in a simple patch and pushed to tryserver.
blocking2.0: ? → final+
Whiteboard: [softblocker]
Assignee

Comment 11

9 years ago
Tryserver has been rather funky these last days - pushed patch again....
Assignee

Comment 12

9 years ago
Posted patch Proposed approach (obsolete) — Splinter Review
The abovementioned patch. Testcase doesn't leak with it's applied and it passes tryserver with 3 oranges related to existing bugs and 2 oranges that can also be found in another run three hours later. I think it's safe. See tryserver run 19b052ddef70 for details (522874338cf8 for the three-hour-later run).

Kyle, I'm not hijacking this bug so I'll leave the patch to you. I have been puzzled by this particular glitch in handling states of cache-entries for some time, but never seen cases where it actually happened.
Feel free to steal the bug, you're much more familiar with this code than I am. :-)
Comment on attachment 504185 [details] [diff] [review]
Proposed approach

Looks good but the comment should be cleaned up. Bjarne, what do you exactly mean with "TODO how to avoid starvation...?" ?
Attachment #504185 - Flags: review?(michal.novotny) → review+
FYI I've created a follow-up bug 627001 for the issue that reload() doesn't finish.
Assignee

Comment 16

9 years ago
(In reply to comment #14)
>  Bjarne, what do you exactly
> mean with "TODO how to avoid starvation...?" ?

Not the best phrasing, I agree...  I meant to say that there is a theoretical chance that we will re-dispatch this request infinitely, and I was wondering if it is worth to deal with it. I don't think there is, and I'll remove the comment.
Bjarne, if you attach an updated patch with the comment fixed I can land this for you (unless someone else beats me to it)...
Assignee

Comment 18

9 years ago
Code identical to the patch r+'ed by Michal. Approved by jst.
Attachment #504185 - Attachment is obsolete: true
Assignee

Comment 19

9 years ago
Requested checkin (not sure what those other flags are for so I'll leave them)
Keywords: checkin-needed

Comment 20

9 years ago
http://hg.mozilla.org/mozilla-central/rev/c3917d4b7a65
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
You need to log in before you can comment on or make changes to this bug.