624217 - "ASSERTION: unexpected request" with <embed> and window.open of same URL

Status: RESOLVED WORKSFORME

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase

On Linux, the testcase triggers:

###!!! ASSERTION: unexpected request: 'Not Reached', file netwerk/protocol/http/nsHttpChannel.cpp, line 3962

I don't have Flash installed.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Bjarne (:bjarne)]

Can reproduce - I'll take a look. It fails on a range-request for a 0-length existing cache-entry (sort-of similar to bug #612135, but the solution for bug #612135 is not right here.)

Comment 3

[Bjarne (:bjarne)]

Attachment: Simple fix which handles the testcase

Resetting this flag should be safe, even for IsFromCache() since |mCachePump| is also reset in CloseCacheEntry(). Pushed to tryserver.

Comment 4

[Bjarne (:bjarne)]

Tryserver-run does not introduce anything new. Awaiting review.

Comment 5

[Bjarne (:bjarne)]

Comment on attachment 502399 [details] [diff] [review]
Simple fix which handles the testcase

Changing reviewer. Honza should know nsHtttChannel pretty good...

Also requesting approval

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 502399 [details] [diff] [review]
Simple fix which handles the testcase

Bjarne, this needs some explanation why we have to drop the flag.  There is none in the bug.  Probably would be good to add an explanatory comment to the patch.  It is safe to do it, but I don't see any reason to do it.

And also, please try to create a test, if it would take a reasonable time to make it.

Comment 7

[Bjarne (:bjarne)]

The test contains two elements referring to the same flash-resource. The first request to the resource is aborted because flash-support is not installed, leaving a 0-size cache-entry.

The second request to the resource finds the cache-entry, sets |mCachedContentIsPartial| and does a range-request. It reads from cache (0 bytes) then calls OnStartRequest eventually getting to nsHttpChannel::CallOnStartRequest(). The listener is nsExternalAppHandler, and its OnStartRequest() calls back and set |mChannelIsForDownload|. A few lines further down in nsHttpChannel::CallOnStartRequest() the cache-entry is doomed and closed.

After some time OnStopRequest() is called, where it asserts because |mCachedContentIsPartial| is still set and |request| != |nsCachePump| since the latter was reset in CloseCacheEntry().

|mCachedContentIsPartial| is normally reset in nsHttpChannel::OnDoneReadingPartialCacheEntry() which is never called in this scenario. Maybe it is worth calling it after dooming the cache-entry, but I believe resetting the flag in CloseCacheEntry() is more efficient and it makes intuitively sense to ensure that the flag is reset at this point.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 502399 [details] [diff] [review]
Simple fix which handles the testcase

Please re-request approval once this is reviewed.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 502399 [details] [diff] [review]
Simple fix which handles the testcase

(In reply to comment #7)
> |mCachedContentIsPartial| is normally reset in
> nsHttpChannel::OnDoneReadingPartialCacheEntry() which is never called in this
> scenario. 

It is not called because |if (request == mCachePump)| is false, what is wrong.

First, I cannot reproduce with the test case.  It is because we do not deploy range requests for zero length cache entries anymore.  See [1] and the condition for range request |size > 0 &&|.

Second, I don't think this is the fix, as I checked after removing the |size > 0| condition.  We enter nsHttpChannel::OnStopRequest called by the cache pump.  It is expected only when:
- we finished reading the content from the cache only (mTransaction is then null)
- we finished reading a partial content (then we end up in OnDoneReadingPartialCacheEntry and early return from OnStopRequest)

With your patch, in the letter case, we close the real transaction and never load the content from the server.

So, I wouldn't call CloseCacheEntry and Doom() as well in CallOnStopRrequest (under |if mChannelIsForDownload|) when we load partial content.  I would postpone it in that case to OnDoneReadingPartialCacheEntry.  The flag should not be changed in CloseCacheEntry.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=508105&action=diff#a/netwerk/protocol/http/nsHttpChannel.cpp_sec1

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: prpopsed fix

Passed just local xpcshell tests and the test case.  No further testing done.

Comment 11

[Bjarne (:bjarne)]

Hmm...  you have a point about this maybe not being an issue anymore after bug #612135 landed (since we don't try to re-use empty cache-entries anymore). Would it be possible to be allowed to see the bugs motivating this one, or maybe reporter could re-test them with the latest code?

About the patches: Your proposal is better, yes. :)  It avoids the situation by not closing the cache-entry in this particular case, hence |mCachePump| is *not* reset, and subsequently |if (request == mCachePump)| becomes true and the "normal" code-path will handle this. This may also make downloading and "Save as" from the page-info/media popup more robust...

Comment 12

[Bjarne (:bjarne)]

I have been trying to create a test for this issue but cannot reproduce it anymore (after reverting the check for size==0 as mentioned in comment #9).

Will resolve as wfm unless reporter still see the original (hidden) bugs. Feel free to cc me on those if further investigation is needed.

Comment 13

[Bjarne (:bjarne)]

Resolving wfm ref comment #12.

Comment 14

[Jesse Ruderman]

WFM