Closed
Bug 442584
Opened 16 years ago
Closed 15 years ago
Prefetching stops working until Firefox is restarted due to a bug in nsPrefetchService::EmptyQueue
Categories
(Core :: Networking: Cache, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: mushhail, Assigned: dcamp)
Details
(Keywords: fixed1.9.1)
Attachments
(1 file)
5.73 KB,
patch
|
jduell.mcbugs
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.10) Gecko/20071115 Firefox/2.0.0.10 Build Identifier: 3.0 Prefetching works a few times, and then stops permanently until the browser is restarted. This happens because nsPrefetchService::EmptyQueue doesn't update mQueueTail, which results in mQueueHead being NULL and mQueueTail being the tail of the old list. All new prefetch requests go to the end of the dead list, and nothing is ever started because the head is NULL. (The root cause of this is implementing and manipulating a linked list directly, instead of using a well-tested container.) The fix is simple: add "mQueueTail = prev" to the end of nsPrefetchService::EmptyQueue in Prefetching works a few times, and then stops permanently until the browser is restarted. This happens because nsPrefetchService::EmptyQueue doesn't update mQueueTail, which results in mQueueHead being NULL and mQueueTail being the tail of the old list. All new prefetch requests go to the end of the dead list, and nothing is ever started because the head is NULL. (The root cause of this is manipulating a linked list directly, instead of using a well-tested container.) The fix is simple: add "mQueueTail = prev" to the end of nsPrefetchService::EmptyQueue in uriloader/prefetch/nsPrefetchService.cpp. Reproducible: Sometimes Steps to Reproduce: normal use
Updated•16 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Prefetching broken (simple fix) → Prefetching stops working until Firefox is restarted due to a bug in nsPrefetchService::EmptyQueue
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → dcamp
Assignee | ||
Comment 1•16 years ago
|
||
Rather than fix up the tail, this patch just reuses DequeueNode() to clean up the list. This patch also has two other fixes that were needed to get the test running: First, 442803 introduced a bug in nsPrefetchQueueEnumerator::Increment() that left the enumerator always reporting empty. Second, we only empty the queue in StopPrefetching() if we've already started prefetching. This patch adds an explicit EmptyQueue() during shutdown and when the pref is disabled.
Attachment #351639 -
Flags: superreview?(cbiesinger)
Attachment #351639 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•15 years ago
|
Flags: blocking1.9.1?
Assignee | ||
Updated•15 years ago
|
Flags: blocking1.9.0.7?
Comment 2•15 years ago
|
||
This sounds bad enough that we should block on it...
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Updated•15 years ago
|
Flags: blocking1.9.0.7? → blocking1.9.0.8?
Comment 3•15 years ago
|
||
Comment on attachment 351639 [details] [diff] [review] trunk fix Jason: adding a review request, seems a good way to get you involved in the code. (We'll still need an official module peer's review though)
Attachment #351639 -
Flags: review?(jduell.mcbugs)
Comment 4•15 years ago
|
||
Why do we need this fix in a 3.0 security update? I guess we could approve a patch (though it sounds like we'll need a separate patch because you don't need to clean up after bug 442803) but we're not blocking on it.
Flags: blocking1.9.0.8?
Comment 5•15 years ago
|
||
Can someone reach out to biesi here?
Comment 6•15 years ago
|
||
Comment on attachment 351639 [details] [diff] [review] trunk fix The logic in the patch looks fine--the only thing that needs to be fixed (?) is that the indentation level in Increment() is out of sync with the rest of the file. This is an exceedingly simple fix (just a couple of obvious data structure bug fixes). I assume we should burn whatever little of biesi's time we get on other things, so I recommend we switch the sr to jst or bz or anyone who's taken a data structures course (or work equivalent)--it's really that simple.
Attachment #351639 -
Flags: review?(jduell.mcbugs) → review+
Comment 7•15 years ago
|
||
Comment on attachment 351639 [details] [diff] [review] trunk fix sr=jst given that Jason reviewd this. biesi, feel free to look if you have cycles, but this can land before biesi gets to this IMO.
Attachment #351639 -
Flags: superreview?(cbiesinger) → superreview+
Updated•15 years ago
|
Whiteboard: needs landing
Assignee | ||
Comment 8•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ef852f0a6ef3
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a312ee7db7c3
Keywords: fixed1.9.1
Whiteboard: needs landing
Comment 10•15 years ago
|
||
While working on an unrelated bug, this test failed: <<<<<<< TEST-UNEXPECTED-FAIL | c:\moz\mozilla-central\obj-i686-pc-mingw32-dbg\_tests\xpcshell\test_docshell\unit\test_bug442584. js | test failed (with xpcshell return code: 3), see following log: >>>>>>> ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to c:\users\ddahl\appdata\local\temp\runxpcshelltests_leaks.log pldhash: for the table at address 022D48B0, the given entrySize of 48 probably favors chaining over double hashing. TEST-INFO | (xpcshell/head.js) | test 1 pending uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJS CID.getService]" nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)" location: "JS frame :: c:\moz\mozilla-centr al\mozilla\testing\xpcshell\head.js :: <TOP_LEVEL> :: line 69" data: no]
Updated•13 years ago
|
Attachment #351639 -
Flags: review?(cbiesinger)
You need to log in
before you can comment on or make changes to this bug.
Description
•