622357 - nsHttpChannel::SetCacheTokenCachedCharset() fails if cache-entry is unavailable

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Bjarne (:bjarne)]

This comes from bug #619571, comment #3

When the cache-entry in nsHttpChannel is fetched asynchronously SetCacheTokenCachedCharset() may be called before it's available and thus fail. In these situations we should store the charset temporarily in nsHttpChannel and transfer it to the cache-entry when it becomes available.

Comment 1

[Bjarne (:bjarne)]

Attachment: Proposed solution

Avoids assertion with this patch - passed to tryserver.

Comment 2

[Bjarne (:bjarne)]

Tryserver run looks fine. About the patch: I guess we should also change GetCacheTokenCachedCharset() to use the temporarily stored charset.

Comment 3

[Honza Bambas (:mayhemer)]

Bjarne, do you have STR for call of SetCacheTokenCachedCharset() while there is no cache entry?

According to this callstack, it shouldn't be possible unless we failed to open the cache entry (then the method should fail, at least silently):

 	xul.dll!nsHTMLDocument::StartDocumentLoad(const char * aCommand=0x63a493c0, nsIChannel * aChannel=0x0a819874, nsILoadGroup * aLoadGroup=0x0a765418, nsISupports * aContainer=0x0a764f58, nsIStreamListener * * aDocListener=0x0ed1cc20, int aReset=1, nsIContentSink * aSink=0x00000000)  Line 944	
	xul.dll!nsContentDLF::CreateDocument(const char * aCommand=0x63a493c0, nsIChannel * aChannel=0x0a819874, nsILoadGroup * aLoadGroup=0x0a765418, nsISupports * aContainer=0x0a764f58, const nsID & aDocumentCID={...}, nsIStreamListener * * aDocListener=0x0ed1cc20, nsIContentViewer * * aDocViewer=0x003acdb8)  Line 467 + 0x33 bytes	
 	xul.dll!nsContentDLF::CreateInstance(const char * aCommand=0x63a493c0, nsIChannel * aChannel=0x0a819874, nsILoadGroup * aLoadGroup=0x0a765418, const char * aContentType=0x0b54dbb8, nsISupports * aContainer=0x0a764f58, nsISupports * aExtraInfo=0x00000000, nsIStreamListener * * aDocListener=0x0ed1cc20, nsIContentViewer * * aDocViewer=0x003acdb8)  Line 255 + 0x25 bytes	
 	xul.dll!nsDocShell::NewContentViewerObj(const char * aContentType=0x0b54dbb8, nsIRequest * request=0x0a819874, nsILoadGroup * aLoadGroup=0x0a765418, nsIStreamListener * * aContentHandler=0x0ed1cc20, nsIContentViewer * * aViewer=0x003acdb8)  Line 7484 + 0x58 bytes	
 	xul.dll!nsDocShell::CreateContentViewer(const char * aContentType=0x0b54dbb8, nsIRequest * request=0x0a819874, nsIStreamListener * * aContentHandler=0x0ed1cc20)  Line 7292 + 0x3e bytes	
 	xul.dll!nsDSURIContentListener::DoContent(const char * aContentType=0x0b54dbb8, int aIsContentPreferred=0, nsIRequest * request=0x0a819874, nsIStreamListener * * aContentHandler=0x0ed1cc20, int * aAbortProcess=0x003ace70)  Line 148 + 0x20 bytes	
 	xul.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener * aListener=0x0a7655d0, nsIChannel * aChannel=0x0a819874)  Line 757 + 0x41 bytes	
 	xul.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest * request=0x0a819874, nsISupports * aCtxt=0x00000000)  Line 455 + 0x39 bytes	
 	xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request=0x0a819874, nsISupports * aCtxt=0x00000000)  Line 295 + 0x10 bytes	
 	xul.dll!nsHttpChannel::CallOnStartRequest()  Line 770 + 0x44 bytes	
 	xul.dll!nsHttpChannel::ContinueProcessNormal(unsigned int rv=0)  Line 1228 + 0x8 bytes	
 	xul.dll!nsHttpChannel::ProcessNormal()  Line 1166	
 	xul.dll!nsHttpChannel::ProcessResponse()  Line 1052 + 0x8 bytes	
 	xul.dll!nsHttpChannel::OnStartRequest(nsIRequest * request=0x09b29540, nsISupports * ctxt=0x00000000)  Line 3876 + 0xe bytes

Comment 4

[Bjarne (:bjarne)]

Ok - here we go

Breakpoint 2, nsHttpChannel::SetCacheTokenCachedCharset (this=0x7fffd6566000, aCharset=...) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4205
4205            return NS_ERROR_NOT_AVAILABLE;
(gdb) bt
#0  nsHttpChannel::SetCacheTokenCachedCharset (this=0x7fffd6566000, aCharset=...) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4205
#1  0x00007ffff538f1fc in nsHTMLDocument::StartDocumentLoad (this=0x7fffd48b4800, aCommand=0x7ffff6d4edde "view", aChannel=0x7fffd6566058, aLoadGroup=0x7fffd485f030, aContainer=0x7fffd6564138, aDocListener=0x7fffd693e4e0, 
    aReset=1, aSink=0x0) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/content/html/document/src/nsHTMLDocument.cpp:944
#2  0x00007ffff4d99386 in nsContentDLF::CreateDocument (this=0x7fffe37fd220, aCommand=0x7ffff6d4edde "view", aChannel=0x7fffd6566058, aLoadGroup=0x7fffd485f030, aContainer=0x7fffd6564138, aDocumentCID=..., 
    aDocListener=0x7fffd693e4e0, aDocViewer=0x7fffffffcd90) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/layout/build/nsContentDLF.cpp:467
#3  0x00007ffff4d986df in nsContentDLF::CreateInstance (this=0x7fffe37fd220, aCommand=0x7ffff6d4edde "view", aChannel=0x7fffd6566058, aLoadGroup=0x7fffd485f030, aContentType=0x7fffd77679c8 "text/html", aContainer=0x7fffd6564138, 
    aExtraInfo=0x0, aDocListener=0x7fffd693e4e0, aDocViewer=0x7fffffffcd90) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/layout/build/nsContentDLF.cpp:255
#4  0x00007ffff5b7555d in nsDocShell::NewContentViewerObj (this=0x7fffd6564000, aContentType=0x7fffd77679c8 "text/html", request=0x7fffd6566058, aLoadGroup=0x7fffd485f030, aContentHandler=0x7fffd693e4e0, aViewer=0x7fffffffcd90)
    at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/docshell/base/nsDocShell.cpp:7477
#5  0x00007ffff5b748e6 in nsDocShell::CreateContentViewer (this=0x7fffd6564000, aContentType=0x7fffd77679c8 "text/html", request=0x7fffd6566058, aContentHandler=0x7fffd693e4e0)
    at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/docshell/base/nsDocShell.cpp:7292
#6  0x00007ffff5b91a00 in nsDSURIContentListener::DoContent (this=0x7fffd693e420, aContentType=0x7fffd77679c8 "text/html", aIsContentPreferred=0, request=0x7fffd6566058, aContentHandler=0x7fffd693e4e0, 
    aAbortProcess=0x7fffffffcf0c) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/docshell/base/nsDSURIContentListener.cpp:148
#7  0x00007ffff5b9a888 in nsDocumentOpenInfo::TryContentListener (this=0x7fffd693e4c0, aListener=0x7fffd693e420, aChannel=0x7fffd6566058)
    at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/uriloader/base/nsURILoader.cpp:757
#8  0x00007ffff5b99468 in nsDocumentOpenInfo::DispatchContent (this=0x7fffd693e4c0, request=0x7fffd6566058, aCtxt=0x0) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/uriloader/base/nsURILoader.cpp:455
#9  0x00007ffff5b989e9 in nsDocumentOpenInfo::OnStartRequest (this=0x7fffd693e4c0, request=0x7fffd6566058, aCtxt=0x0) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/uriloader/base/nsURILoader.cpp:295
#10 0x00007ffff4c2d28b in nsHttpChannel::CallOnStartRequest (this=0x7fffd6566000) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:770
#11 0x00007ffff4c2e8ad in nsHttpChannel::ContinueProcessNormal (this=0x7fffd6566000, rv=2147500037) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:1228
#12 0x00007ffff4c2e597 in nsHttpChannel::ProcessNormal (this=0x7fffd6566000) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:1165
#13 0x00007ffff4c2e07f in nsHttpChannel::ProcessResponse (this=0x7fffd6566000) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:1052
#14 0x00007ffff4c39a4c in nsHttpChannel::OnStartRequest (this=0x7fffd6566000, request=0x7fffd4879200, ctxt=0x0) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:3877
#15 0x00007ffff4b2eafa in nsInputStreamPump::OnStateStart (this=0x7fffd4879200) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:441
#16 0x00007ffff4b2e92a in nsInputStreamPump::OnInputStreamReady (this=0x7fffd4879200, stream=0x7fffd51c26b8) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:397
#17 0x00007ffff62e0b41 in nsInputStreamReadyEvent::Run (this=0x7fffd8c295c0) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/xpcom/io/nsStreamUtils.cpp:112
#18 0x00007ffff630e9cd in nsThread::ProcessNextEvent (this=0x7fffeaf2a100, mayWait=0, result=0x7fffffffd6ec) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/xpcom/threads/nsThread.cpp:626
#19 0x00007ffff62913e8 in NS_ProcessNextEvent_P (thread=0x7fffeaf2a100, mayWait=0) at nsThreadUtils.cpp:250
#20 0x00007ffff60c57c4 in mozilla::ipc::MessagePump::Run (this=0x7fffeaf486c0, aDelegate=0x7fffeafa9840) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/ipc/glue/MessagePump.cpp:110
#21 0x00007ffff637d9c1 in MessageLoop::RunInternal (this=0x7fffeafa9840) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/ipc/chromium/src/base/message_loop.cc:219
#22 0x00007ffff637d946 in MessageLoop::RunHandler (this=0x7fffeafa9840) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/ipc/chromium/src/base/message_loop.cc:202
#23 0x00007ffff637d8d7 in MessageLoop::Run (this=0x7fffeafa9840) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/ipc/chromium/src/base/message_loop.cc:176
#24 0x00007ffff5f4182f in nsBaseAppShell::Run (this=0x7fffeafce970) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:192
#25 0x00007ffff5c60d8d in nsAppStartup::Run (this=0x7fffe37b2330) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/toolkit/components/startup/src/nsAppStartup.cpp:191
#26 0x00007ffff4ad6879 in XRE_main (argc=5, argv=0x7fffffffe1b8, aAppData=0x7fffeaf2b080) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/toolkit/xre/nsAppRunner.cpp:3694
#27 0x0000000000401fc2 in main (argc=5, argv=0x7fffffffe1b8) at /home/bjarne/Work/Mozilla/CleanFireFox/mozilla-central/browser/app/nsBrowserApp.cpp:158
(gdb) p *this
$1 = {<mozilla::net::HttpBaseChannel> = {<nsHashPropertyBag> = {<nsIWritablePropertyBag> = {<nsIPropertyBag> = {<nsISupports> = {
            _vptr.nsISupports = 0x7ffff791c0d0}, <No data fields>}, <No data fields>}, <nsIWritablePropertyBag2> = {<nsIPropertyBag2> = {<nsIPropertyBag> = {<nsISupports> = {
              _vptr.nsISupports = 0x7ffff791c558}, <No data fields>}, <No data fields>}, <No data fields>}, mRefCnt = {mValue = 14}, _mOwningThread = {mThread = 0x7fffeaf14040}, 
      mPropertyHash = {<nsBaseHashtable<nsStringHashKey, nsCOMPtr<nsIVariant>, nsIVariant*>> = {<nsTHashtable<nsBaseHashtableET<nsStringHashKey, nsCOMPtr<nsIVariant> > >> = {mTable = {ops = 0x7ffff7b3c5c0, data = 0x0, 
              hashShift = 28, maxAlphaFrac = 192 '\300', minAlphaFrac = 64 '@', entrySize = 32, entryCount = 2, removedCount = 0, generation = 0, 
              entryStore = 0x7fffd6566400 "H\027\350\v"}}, <No data fields>}, <No data fields>}}, <nsIEncodedChannel> = {<nsISupports> = {
        _vptr.nsISupports = 0x7ffff791c640}, <No data fields>}, <nsIHttpChannel> = {<nsIChannel> = {<nsIRequest> = {<nsISupports> = {
            _vptr.nsISupports = 0x7ffff791c680}, <No data fields>}, <No data fields>}, <No data fields>}, <nsIHttpChannelInternal> = {<nsISupports> = {
        _vptr.nsISupports = 0x7ffff791c810}, <No data fields>}, <nsIUploadChannel> = {<nsISupports> = {_vptr.nsISupports = 0x7ffff791c890}, <No data fields>}, <nsIUploadChannel2> = {<nsISupports> = {
        _vptr.nsISupports = 0x7ffff791c8c8}, <No data fields>}, <nsISupportsPriority> = {<nsISupports> = {_vptr.nsISupports = 0x7ffff791c8f8}, <No data fields>}, <nsIResumableChannel> = {<nsISupports> = {
        _vptr.nsISupports = 0x7ffff791c938}, <No data fields>}, mURI = {mRawPtr = 0x7fffd7726200}, mOriginalURI = {mRawPtr = 0x7fffd7726200}, mDocumentURI = {mRawPtr = 0x7fffd8c13300}, mListener = {mRawPtr = 0x7fffd693e4c0}, 
    mListenerContext = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}, mLoadGroup = {mRawPtr = 0x7fffd485f030}, mOwner = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}, mCallbacks = {mRawPtr = 0x7fffd6564030}, 
    mProgressSink = {mRawPtr = 0x7fffd6564020}, mReferrer = {mRawPtr = 0x7fffdaa72800}, mApplicationCache = {mRawPtr = 0x0}, mRequestHead = {mHeaders = {mHeaders = {<nsTArray_base<nsTArrayDefaultAllocator>> = {
            mHdr = 0x7fffd4edb3d0}, <No data fields>}}, mMethod = {_val = 0x7ffff6a4aee7 "GET"}, mVersion = 11 '\v', mRequestURI = {<nsACString_internal> = {
          mData = 0x7fffd4ed9598 "/plugins/likebox.php?id=120183438017841&width=320&connections=10&stream=false&header=false&height=255", mLength = 101, mFlags = 5}, <No data fields>}}, mUploadStream = {mRawPtr = 0x0}, 
    mResponseHead = {mRawPtr = 0x7fffd48eb5b0}, mConnectionInfo = {mRawPtr = 0x7fffd7a61ec0}, mSpec = {<nsACString_internal> = {
        mData = 0x7fffd485ef88 "http://www.facebook.com/plugins/likebox.php?id=120183438017841&width=320&connections=10&stream=false&header=false&height=255", mLength = 124, mFlags = 5}, <No data fields>}, mContentTypeHint = warning: can't find linker symbol for virtual table for `nsCString' value
warning:   found `EmptyString()::sEmpty' instead

{<nsACString_internal> = {mData = 0x7ffff7b984b0 "", mLength = 0, mFlags = 1}, <No data fields>}, mContentCharsetHint = warning: can't find linker symbol for virtual table for `nsCString' value
warning:   found `EmptyString()::sEmpty' instead
{<nsACString_internal> = {mData = 0x7ffff7b984b0 "", mLength = 0, mFlags = 1}, <No data fields>}, 
    mUserSetCookieHeader = warning: can't find linker symbol for virtual table for `nsCString' value
warning:   found `EmptyString()::sEmpty' instead
{<nsACString_internal> = {mData = 0x7ffff7b984b0 "", mLength = 0, mFlags = 1}, <No data fields>}, mEntityID = warning: can't find linker symbol for virtual table for `nsCString' value
warning:   found `EmptyString()::sEmpty' instead
{<nsACString_internal> = {mData = 0x7ffff7b984b0 "", mLength = 0, 
        mFlags = 1}, <No data fields>}, mStartPos = 18446744073709551615, mStatus = 0, mLoadFlags = 7405824, mPriority = 0, mCaps = 1 '\001', mRedirectionLimit = 20 '\024', mApplyConversion = 1, mCanceled = 0, mIsPending = 1, 
    mWasOpened = 1, mResponseHeadersModified = 0, mAllowPipelining = 1, mForceAllowThirdPartyCookie = 0, mUploadStreamHasHeaders = 0, mInheritApplicationCache = 0, mChooseApplicationCache = 0, mLoadedFromApplicationCache = 0, 
    mChannelIsForDownload = 0}, <nsIStreamListener> = {<nsIRequestObserver> = {<nsISupports> = {
        _vptr.nsISupports = 0x7ffff791c970}, <No data fields>}, <No data fields>}, <nsICachingChannel> = {<nsICacheInfoChannel> = {<nsISupports> = {
        _vptr.nsISupports = 0x7ffff791c9b0}, <No data fields>}, <No data fields>}, <nsICacheListener> = {<nsISupports> = {_vptr.nsISupports = 0x7ffff791ca60}, <No data fields>}, <nsITransportEventSink> = {<nsISupports> = {
      _vptr.nsISupports = 0x7ffff791ca90}, <No data fields>}, <nsIProtocolProxyCallback> = {<nsISupports> = {
      _vptr.nsISupports = 0x7ffff791cac0}, <No data fields>}, <nsIHttpAuthenticableChannel> = {<nsIProxiedChannel> = {<nsISupports> = {
        _vptr.nsISupports = 0x7ffff791caf0}, <No data fields>}, <No data fields>}, <nsITraceableChannel> = {<nsISupports> = {
      _vptr.nsISupports = 0x7ffff791cb98}, <No data fields>}, <nsIApplicationCacheChannel> = {<nsIApplicationCacheContainer> = {<nsISupports> = {
        _vptr.nsISupports = 0x7ffff791cbc8}, <No data fields>}, <No data fields>}, <nsIAsyncVerifyRedirectCallback> = {<nsISupports> = {_vptr.nsISupports = 0x7ffff791cc30}, <No data fields>}, mSecurityInfo = {<nsCOMPtr_base> = {
      mRawPtr = 0x0}, <No data fields>}, mProxyRequest = {mRawPtr = 0x0}, mTransactionPump = {mRawPtr = 0x7fffd4879200}, mTransaction = {mRawPtr = 0x7fffd52d3010}, mLogicalOffset = 0, mCacheEntry = {mRawPtr = 0x0}, mCachePump = {
    mRawPtr = 0x0}, mCachedResponseHead = {mRawPtr = 0x0}, mCacheAccess = 0, mPostID = 0, mRequestTime = 1294131592, mOnCacheEntryAvailableCallback = NULL, mAsyncCacheOpen = 1, mOfflineCacheEntry = {mRawPtr = 0x0}, 
  mOfflineCacheAccess = -1515870811, mOfflineCacheClientID = {<nsACString_internal> = {mData = 0x7ffff7b984b0 "", mLength = 0, mFlags = 1}, <No data fields>}, mAuthProvider = {mRawPtr = 0x0}, mPendingAsyncCallOnResume = NULL, 
  mTargetProxyInfo = {mRawPtr = 0x0}, mSuspendCount = 0, mFallbackKey = {<nsACString_internal> = {mData = 0x7ffff7b984b0 "", mLength = 0, mFlags = 1}, <No data fields>}, mRedirectURI = {mRawPtr = 0x0}, mRedirectChannel = {
    mRawPtr = 0x0}, mRedirectType = 2779096485, mCachedContentIsValid = 0, mCachedContentIsPartial = 0, mTransactionReplaced = 0, mAuthRetryPending = 0, mResuming = 0, mInitedCacheEntry = 0, mCacheForOfflineUse = 0, 
  mCachingOpportunistically = 0, mFallbackChannel = 0, mTracingEnabled = 0, mCustomConditionalRequest = 0, mFallingBack = 0, mWaitingForRedirectCallback = 0, mRequestTimeInitialized = 1, mRedirectFuncStack = warning: can't find linker symbol for virtual table for `nsTArray<nsresult (nsHttpChannel::*)(nsresult), nsTArrayDefaultAllocator>' value
warning:   found `EmptyEnumeratorImpl::kInstance' instead

{<nsTArray_base<nsTArrayDefaultAllocator>> = {mHdr = 0x7ffff7b97e88}, <No data fields>}, mHasher = {mRawPtr = 0x0}}
(gdb)  

Note values of |mAsyncCacheOpen| and |mCacheEntry|.

However, even if we do fail to open a cache-entry: Do we really want to take the consequence from bug #619571, comment #8 ? Shouldn't we rather apply some workaround, like the one proposed here?

Comment 5

[Honza Bambas (:mayhemer)]

As I understand (after mxr search):
- in nsHTMLDocument::StartDocumentLoad we are trying to determine char set of the page being loaded
- we try to get this information e.g. from the http channel (Content-type header), then try to get it from the cache (our case), then try a default charset set by user, etc etc..
- after we determine it, in the same method, we store it to the cache with SetCacheTokenCachedCharset
- on next load we do the same, so I'm not sure (probably I'm missing something) how can we get to state described in bug #619571, comment #8 - we just simply don't have the charset cached but we then use other ways to determined as in case we load the page for the first time

Also, I don't think the char set will be stored anywhere even with your patch.  I don't think we get a cache entry anytime later.

If you just want to store the cached char set somewhere for the lifetime of the channel then yes, the patch is quit correct.  But as described above (if I don't miss anything) it's useless.

Comment 6

[Bjarne (:bjarne)]

(In reply to comment #5)
> Also, I don't think the char set will be stored anywhere even with your patch. 
> I don't think we get a cache entry anytime later.

You're right: I checked in the debugger and the cache-entry never appears. I was simply (and wrongly) assuming that since |mAsyncCacheOpen| was set we were actually just waiting for the entry.

> If you just want to store the cached char set somewhere for the lifetime of the
> channel then yes,

Which brings us back to the original issue, namely the assertion in nsHTMLDocument. Bz: Could it be removed? Or should I dig a little deeper and try to figure out why the entry is not created and possibly how to handle it? (In other words: How crucial is storing the charset.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> we then use other ways to determined as in case we load the page for the first
> time

You're assuming that there are only two loads.  The case this is supposed to handle has _3_ loads.  First load, we detect the charset incorrectly.  User overrides using the view menu; we do a second load with the override, and store that charset in the cache entry.  Third load, we get it right because the charset is in the cache entry, so the user doesn't have to keep doing the manual override thing.

> Or should I dig a little deeper and try to figure out why the entry is not
> created

Not created, or already closed by the time we get into this code?

> (In other words: How crucial is storing the charset.)

For western stuff, not terribly.  As I understand in some other locales it's more common to have misdetection issues (esp. since the browser default charset is ISO-8859-1, which works for western stuff, but not other things).

Comment 8

[Bjarne (:bjarne)]

(In reply to comment #7)
> > Or should I dig a little deeper and try to figure out why the entry is not
> > created
> 
> Not created, or already closed by the time we get into this code?

Well, that might be revealed by abovementioned digging...  ;)

> > (In other words: How crucial is storing the charset.)
> 
> For western stuff, not terribly.  As I understand in some other locales it's
> more common to have misdetection issues (esp. since the browser default charset
> is ISO-8859-1, which works for western stuff, but not other things).

Ok - I'll dig some more and see what pops up. Are you aware of any tests for the 3-load-case mentioned?

Comment 9

[Bjarne (:bjarne)]

In this particular case we actually fail to initialize the cache-entry in ContinueProcessNormal() and hence close it...  I'll dig a little more to figure out why this happens.

Comment 10

[Bjarne (:bjarne)]

Finally got around to check this: It turns out that it is nsCacheService::IsStorageEnabledForPolicy() which fails, causing nsHttpChannel::InitCacheEntry() to fail and hence nsHttpChannel::ContinueProcessNormal() to close the cache-entry before calling OnStartRequest.

The reason nsCacheService::IsStorageEnabledForPolicy() fails is that I (by accident) had memory-cache disabled in the profile I used, and the resource loaded has a cache-control: no-store set. This causes FF to try storing it in memory (for history-reasons), but since I accidentally had the mem-cache disabled, it failed.

Even though this case turned out to be rather obscure, the original issue still remains. IMO it is wrong to assume that nsHttpChannel::SetCacheTokenCachedCharset() always succeeds (in fact, this case shows that it may fail for perfectly valid reasons), and I'd suggest to simply remove the assertion in nsHTMLDocument. It means that in certain situations we will not be able to cache and re-use overridden charsets, but must rely on other mechanisms to figure it out.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I think removing the assert (and replacing it with a warning) is fine.

Comment 12

[Bjarne (:bjarne)]

Attachment: Another approach, replacing the assertion with warning

It also seems like this is the only place this method is used.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 502258 [details] [diff] [review]
Another approach, replacing the assertion with warning

Please re-request approval for this patch once it's reviewed, if you still think we should take this for 2.0, that is.

Comment 14

[Bjarne (:bjarne)]

Please note that handling this is a prerequisite for landing the proposed solution for bug #630420 because the latter will trigger lots of cases with unavailable cache-entries.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 502258 [details] [diff] [review]
Another approach, replacing the assertion with warning

r=me

Comment 16

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/cb1d77b085de