Prefetching stops working until Firefox is restarted due to a bug in nsPrefetchService::EmptyQueue

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
Networking: Cache
P2
major
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: John, Assigned: dcamp)

Tracking

({fixed1.9.1})

unspecified
mozilla1.9.1
x86
Windows XP
fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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

10 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

10 years ago
Assignee: nobody → dcamp
(Assignee)

Comment 1

10 years ago
Created attachment 351639 [details] [diff] [review]
trunk fix

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

9 years ago
Flags: blocking1.9.1?
(Assignee)

Updated

9 years ago
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+

Updated

9 years ago
Whiteboard: needs landing
(Assignee)

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/ef852f0a6ef3
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a312ee7db7c3
Keywords: fixed1.9.1
Whiteboard: needs landing

Comment 10

9 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]
You need to log in before you can comment on or make changes to this bug.