303003 - ASSERTION: FindEntry() called for a bound entry.: '!binding'

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Bob Clary [:bc] (inactive)]

Found using a CVS debug build from 7/31 on WinXP.

ASSERTION: FindEntry() called for a bound entry.: '!binding' occured 13 times
during top site testing of 5469 pages. It appears to be new although I don't
have good history of logs for full top site testing.

I doubt this is related to the splitwindow bug, but ccing jst just in case. 

The urls this appeared in are (* reproduced loading page by itself)
<http://www.amazon.com/exec/obidos/am/103-8516287-7640622?p=http%3A%2F%2Fwww.amazon.com%2Fgp%2Fsearch.html%2F%3F%26brand%3DSeiko%26keywords%3DKinetic%26node%3D3888811&l=46663901&c=46663901&i=2498679043&hmac=FFF2C2DE125F20E4C3C1F88CA9A01D091FDBC057>
<http://www.netscape.com/redir.adp?_dci_url=http%3a%2f%2fchannels%2enetscape%2ecom%2fns%2fatplay%2fdefault%2ejsp&_wps_s=nb%5fll1%5fu%5f8>
*<http://www.netscape.com/redir.adp?_dci_url=http%3a%2f%2fchannels%2enetscape%2ecom%2fns%2fpf%2fstocks%2ejsp&_wps_s=mktbl%5flll1%5fu0%5f1>
*<http://www.netscape.com/redir.adp?_dci_url=http%3a%2f%2fchannels%2enetscape%2ecom%2fns%2fpf%2fdetailed%5fquote%2ejsp%3fTickerSymbols%3d%24indu&_wps_s=6%5fdow>
*<http://www.netscape.com/redir.adp?_dci_url=http%3a%2f%2fchannels%2enetscape%2ecom%2fns%2fpf%2fdetailed%5fquote%2ejsp%3fTickerSymbols%3d%24inx&_wps_s=6%5fsp>

Comment 1

[Darin Fisher]

-> default owner

Comment 2

[Martijn Wargers (dead)]

Attachment: Backtrace from debug build

I can't see the assertion with the url's mentioned in comment 0.
But I can see the assertion on first load or shift-reload on http://www.lidl.nl/nl/home.nsf/pages/c.o.20060807.index
I've attached a backtrace of the assertion, it seems that in this case the assertion is happening while loading an image.

Comment 3

[Radek 'sysKin' Czyz]

This is because assertion checks if entry is active, but only bases its check on hash. Two cache entries with identical hash can exist (it's handled correctly) but assert will still fire.

Steps to reproduce:
1. Visit www.spaceflightnow.com
2. Visit it again or do view->page info

Actual results:
Assert fires because the following objects have the same hash:
http://spaceflightnow.com/news/images/ni0705/070516apollo17_140.jpg
http://spaceflightnow.com/news/images/ni0705/070511apollo10_140.jpg

Expected results:
Assert confirms that entries differ and doesn't fire

Quite evil isn't it? I made a patch, attaching....

Comment 4

[Radek 'sysKin' Czyz]

Attachment: don't fire wrong assert

This is my first mozilla patch ever :)

Biesi I ask you for review because you said tonight: 02:27:25 : <biesi> sysKin_away, cache should really not assert

:)

Comment 5

[Boris Zbarsky [:bzbarsky]]

I wonder whether we could come up with a better hash setup such that those wouldn't collide.  That's got to be a good idea, no?  File a followup bug?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Put another way, why are we not using an actual hashtable instead of just finding by hash number?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

because we name the actual disk cache files after the hash number, I would assume.

I have no idea whether the patch is correct though... you should ask alfredkayser for review instead.

Comment 8

[Alfred Kayser]

Patch looks ok to me.

Indeed hash collisions can happen and are handled (but not in a very strong way).

The hash collision problem itself can be addressed in two ways:
* switch to the SQL based cache for the diskcache
* extend the hash key (it is used as filename, but also stored as a 32bit integer), which has big impact on cache structure.

Comment 9

[Radek 'sysKin' Czyz]

Comment on attachment 268079 [details] [diff] [review]
don't fire wrong assert

Great, could you please make it r+ and commit please? :)

Risk is none, as it's debug-only.

Comment 10

[Alfred Kayser]

Comment on attachment 268079 [details] [diff] [review]
don't fire wrong assert

I can r+ plus it, but I don't have checkin access

Comment 11

[Martijn Wargers (dead)]

Doesn't the patch need supperreview?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I checked this in.