Closed Bug 1182487 Opened 4 years ago Closed 4 years ago

INHIBIT_CACHING causing assertion

Categories

(Core :: Networking: HTTP, defect)

41 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: valentin, Assigned: mayhemer)

References

Details

Attachments

(2 files, 1 obsolete file)

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)
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Not sure if I'm doing something wrong, or it just regressed at some point.
Assignee: valentin.gosu → nobody
Status: ASSIGNED → NEW
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.
Flags: needinfo?(honzab.moz)
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)
Flags: needinfo?(honzab.moz) → needinfo?(valentin.gosu)
(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 :)
Flags: needinfo?(valentin.gosu)
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!
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
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
Attachment #8634344 - Flags: review?(michal.novotny)
Attachment #8632108 - Attachment is obsolete: true
Assignee: honzab.moz → valentin.gosu
bzexport changed the asignee again :)
Assignee: valentin.gosu → honzab.moz
Attachment #8634616 - Flags: review?(michal.novotny) → review+
Attachment #8634344 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/b2ca921b8827
https://hg.mozilla.org/mozilla-central/rev/54e51956cd9f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.