1182487 - INHIBIT_CACHING causing assertion

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Valentin Gosu [:valentin] (he/him)]

When opening a channel with the nsIRequest.INHIBIT_CACHING flag set, an assertion occurs at:

"Assertion failure: mCacheEntryIsWriteOnly || mCachedContentIsPartial, at /home/icecold/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4233"
"#01: mozilla::net::nsHttpChannel::InstallCacheListener(long) (/home/icecold/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4233 (discriminator 7)

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Test case for opening channel with nsIRequest::INHIBIT_CACHING flag set

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Not sure if I'm doing something wrong, or it just regressed at some point.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Honza, this assertion was added by you in rev 148125 (bug 913807)
Does it still make sense, or did our assumptions change over time?
Everything seems to work just fine if I remove it.

Comment 4

[Honza Bambas (:mayhemer)]

So, the assertion seems correct.  InstallCacheListener is preparing write to the cache entry.  Hence one of this MUST be true.

Which part is failing?  mCacheEntryIsWriteOnly or mCachedContentIsPartial?

I'll try the test but I don't follow your comment 2 (where you disassign yourself from this bug)

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> So, the assertion seems correct.  InstallCacheListener is preparing write to
> the cache entry.  Hence one of this MUST be true.

Shouldn't we skip writing to the cache entry if INHIBIT_CACHING is present?

> 
> Which part is failing?  mCacheEntryIsWriteOnly or mCachedContentIsPartial?
> 
> I'll try the test but I don't follow your comment 2 (where you disassign
> yourself from this bug)

I was just confused that what seems as a basic use case triggers the assert :)

Comment 6

[Honza Bambas (:mayhemer)]

This is there since ever.  The condition to call InstallCacheListener is simply wrong!

Valentin, don't try to remove assertions you don't see behind!

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: 1182487-bad-install-cache-listener-cond.patch

mLoadedFromApplicationCache == mCacheEntryIsReadOnly at this place, all the time, so it's mostly a no-change for that case.  Basic xpcshell test run for network passes.

Valentin: the test case should be incorporated, just needs some cleanup of what is commented out and not according standards.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82cc31e492c2

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Add test for opening the channel with nsIRequest::INHIBIT_CACHING flag set

Comment 9

[Valentin Gosu [:valentin] (he/him)]

bzexport changed the asignee again :)

Comment 10

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2b81755edd0

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ca921b8827
https://hg.mozilla.org/integration/mozilla-inbound/rev/54e51956cd9f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b2ca921b8827
https://hg.mozilla.org/mozilla-central/rev/54e51956cd9f