110405 - Assertions in cache code

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Simon Fraser [no longer active]]

Over the last couple of days, I've been getting these assertions from disk cache:
###!!! ASSERTION: oops.. bad flags: 'entry->IsAllowedInMemory()', file 
nsCacheService.cpp, line 939

Comment 1

[Simon Fraser [no longer active]]

Stack for the assertion:
 Calling chain using A6/R1 links
  Back chain  ISA  Caller
  00000000    PPC  3D9EB09C  
  07679570    PPC  3D9CAEEC  main+001B0
  07679510    PPC  3D9C8984  main1(int, char**, nsISupports*)+00A78
  076793E0    PPC  3D5A5390  nsAppShellService::Run()+00054
  07679390    PPC  3A28BD68  nsAppShell::Run()+0004C
  07679350    PPC  3A28CA04  nsMacMessagePump::DoMessagePump()+00044
  07679300    PPC  3A28CEBC  nsMacMessagePump::DispatchEvent(int, EventRecord*)+
001B0
  076792B0    PPC  3D75FED4  Repeater::DoRepeaters(const EventRecord&)+0003C
  07679260    PPC  3A274300  nsMacNSPREventQueueHandler::RepeatAction(const 
EventRecord&)+000
14
  07679220    PPC  3A274910  nsMacNSPREventQueueHandler::ProcessPLEventQueue()+
00180
  076791A0    PPC  3D80BFC8  nsEventQueueImpl::ProcessPendingEvents()+00068
  07679130    PPC  3D884ECC  PL_ProcessPendingEvents+000BC
  076790E0    PPC  3D885120  PL_HandleEvent+00054
  076790A0    PPC  3C6503FC  HandleImageOnerrorPLEvent(PLEvent*)+00030
  07679060    PPC  3C65012C  HandleImagePLEvent(nsIContent*, unsigned int, 
unsigned int)+005A
4
  07678F50    PPC  3CFEAC08  nsXULElement::HandleDOMEvent(nsIPresContext*, 
nsEvent*, nsIDOMEv
ent**, unsigned int, nsEventStatus*)+01D28
  07678A10    PPC  3CCBA958  nsEventListenerManager::HandleEvent(nsIPresContext*, 
nsEvent*, n
sIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*)+026CC
  07678540    PPC  3CCB7EF0  
nsEventListenerManager::HandleEventSubType(nsListenerStruct*, ns
IDOMEvent*, nsIDOMEventTarget*, unsigned int, unsigned int)+00A18
  07678300    PPC  3CAD11A0  nsJSEventListener::HandleEvent(nsIDOMEvent*)+00CA4
  07678100    PPC  3CA9783C  nsJSContext::CallEventHandler(void*, void*, unsigned 
int, void*,
 int*, int)+0041C
  07678020    PPC  3D5FD8D8  JS_CallFunctionValue+00044
  07677FE0    PPC  3D624CA8  js_InternalInvoke+000D0
  07677F20    PPC  3D6249AC  js_Invoke+00A90
  07677E20    PPC  3D632A18  js_Interpret+0CE50
  07677A20    PPC  3D624938  js_Invoke+00A1C
  07677920    PPC  3A202438  XPC_WN_CallMethod(JSContext*, JSObject*, unsigned 
int, long*, lo
ng*)+001A8
  07677830    PPC  3A1F8590  XPCWrappedNative::CallMethod(XPCCallContext&, 
XPCWrappedNative::
CallMode)+0147C
  076774D0    PPC  3D83B90C  XPTC_InvokeByIndex+0002C
  07677490    PPC  3D83BA18  _XPTC_InvokeByIndex+000CC
  076773E0    PPC  3D57B070  nsCacheEntryDescriptor::MarkValid()+00034
  076773A0    PPC  3D585E84  nsCacheService::ValidateEntry(nsCacheEntry*)+00090
  07677330    PPC  3D585D28  nsCacheService::EnsureEntryHasDevice(nsCacheEntry*)+
00154
  076772E0    PPC  3D7DF67C  nsDebug::Assertion(const char*, const char*, const 
char*, int)+0

Comment 2

[gordon]

Simon, are you trying out hyatt's new icon caching?

Comment 3

[Simon Fraser [no longer active]]

Nope. No special settings in my build.

Comment 4

[gordon]

Looking at the stack, something seems to be calling the cache service from
javascript.  Hyatt's new code was the only thing that I was aware of that did that.

Comment 5

[Simon Fraser [no longer active]]

I guess the favicon stuff defaults to 'on'.

Comment 6

[David Hyatt]

What is the assertion?

Comment 7

[gordon]

The assertion is at:

http://lxr.mozilla.org/seamonkey/source/netwerk/cache/src/nsCacheService.cpp#939

Simon had his disk cache disabled, and the icon cache only wants to store stuff
on disk.  Currently it appears we will store your entries in the memory cache if
the disk cache is disabled.

The assert was originally put in to catch misuses of the cache by known (at the
time) clients.  It now appears to be anachronistic.

I'll submit a patch to fix this.

Comment 8

[gordon]

Attachment: remove anachronistic assert and respect memory storage policy.

Comment 9

[gordon]

Simon, Darin, Hyatt, can I get an r and sr on this?

Hyatt, are you sure you don't want to do any caching if the disk cache is disabled?

Comment 10

[Simon Fraser [no longer active]]

Looks OK to me, r/sr=sfraser

Comment 11

[Darin Fisher]

Comment on attachment 58176 [details] [diff] [review]
remove anachronistic assert and respect memory storage policy.

r/sr=darin

Comment 12

[David Hyatt]

Attachment: Patch to change the storage policy of the icon missed cache.

Comment 13

[David Hyatt]

Comment on attachment 58176 [details] [diff] [review]
remove anachronistic assert and respect memory storage policy.

sr=hyatt on gordon's patch.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 58583 [details] [diff] [review]
Patch to change the storage policy of the icon missed cache.

STORE_ANYWHERE does the right thing in the current impl, but its definition is
a little vague.  Gordon, any chance of firming that up, or else providing a
STORE_ANYWHERE_PREFER_DISK cookie?

/be

Comment 15

[Darin Fisher]

STORE_ANYWHERE is meant to be vague.  it means that the client of the cache
doesn't really care where the data ends up.

we currently interpret this to mean that the data should go straight to disk,
unless the disk cache is disabled, in which case we put the data in the memory
cache.  but, i don't think hyatt requires this behavior.

Comment 16

[Brendan Eich [:brendan]]

We're not sure what's required for good favicon negative caching, it depends on
how often a user restarts, and perhaps on cache sizes (but probably not, for any
big-enough quota -- if the site is hit more than once per session, its negative
favicon entry should stay recently-used and not be evicted unless lots of other
cache traffic blows it away).

But disk is best for negative cache entry lifetime, because it persists across
app sessions, and because its quote is greatest.  I'm not worried, but if I saw
implementation code that preferred the memory cache for STORE_ANYWHERE, I might
then want to measure first.

/be

Comment 17

[David Hyatt]

Yes, I absolutely prefer disk, since I would like the data to always persist
across sessions if possible.  

Comment 18

[Darin Fisher]

yeah, back when we came up with the storage policy flags, we went back and forth
on the names.  at one point, we discussed emphasizing persistence, but we ended
up with the current set of names, because there simply may not always be a way
to persist cache entries.  so, we decided that STORE_ANYWHERE would mean that
persistence is OK.

perhaps if we ever end up with an aggresive 2 level cache, we might want to
introduce a storage policy that puts stronger emphasis on persistence with a
fallback on memory only.  however, its very unlikely that we'd ever write a 2
level cache ourselves... more than likely, our 2 level cache will actually
result from using memory mapped files for disk i/o.  in which, case the current
set of storage policy flags would still apply nicely.

Comment 19

[gordon]

If/When we need to add additional cache devices (besides just disk and memory),
we will need to revisit the naming of the storage policy constants.  Our
original vision, with drop-in devices, was much more ambitious and too grandiose
for the 6.1 timeframe.  It was meant as a statement of future direction, and the
current storage policy constants were a practical compromise given the (then)
current needs of the cache clients.  I'm not worried about STORE_ANYWHERE
getting redefined.  When the day comes that we need a new storage policy, we'll
probably want to change a lot more than just the semantics of STORE_ANYWHERE.

This seems like an odd place to have this discussion... :-)

Comment 20

[gordon]

patches checked in. Marking FIXED.