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)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: mushhail, Assigned: dcamp)

Details

(Keywords: fixed1.9.1)

Attachments

(1 file)

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
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: nobody → dcamp
Attached patch trunk fixSplinter Review
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)
Flags: blocking1.9.1?
Flags: blocking1.9.0.7?
This sounds bad enough that we should block on it...
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Flags: blocking1.9.0.7? → blocking1.9.0.8?
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)
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?
Can someone reach out to biesi here?
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 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+
Whiteboard: needs landing
http://hg.mozilla.org/mozilla-central/rev/ef852f0a6ef3
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: