http-on-examine-response isn't fired when a response comes from the cache

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Networking
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Honza, Assigned: michal)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
blocking1.9.0.9 -
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [firebug-p2])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

10 years ago
If a response is loaded directly from the cache, without any communication with the server, the http-on-examine-response event isn't fired. 

There are two different cases when the response comes from the cache:

1) The response is directly loaded from the cache without any network communication with the server. The event isn't fired in this case - this is the bug.

2) Status 304 Not Modified is received from the server and the response is consequently loaded from the cache. The event is properly fired in this case. and there is also http-on-examine-merged-response event fired.

There should be also a flag that allows to distinguish both cases. 

The missing event makes difficult to monitor network communication in Firebug's Net panel as there is no hint that a network request has been already finished.
maybe 1) should just be a separate event. that avoids the flag. (it also avoids potential backwards compatibility problems, although I can't think of any right now)
(Reporter)

Comment 2

10 years ago
Yep, separate event would work for me.

Updated

10 years ago
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
(Reporter)

Comment 3

10 years ago
Following Firebug issues depends on this bug:
http://code.google.com/p/fbug/issues/detail?id=1029
http://code.google.com/p/fbug/issues/detail?id=933

There is no test case, but probably also:
http://code.google.com/p/fbug/issues/detail?id=1038

It would be really awesome to have the event, thanks!

Honza




this would be really useful for firebug going forward. Does anybody have any cycles to take a look at it?
Whiteboard: [firebug-p2]
Flags: blocking1.9.1?
this could really use an owner.
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Updated

10 years ago
Assignee: nobody → michal
Per the issue links in comment 3, it looks like Firebug has worked around this? Not going to block based on that, but we really should make life easier for Firebug.
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
(Reporter)

Comment 7

10 years ago
Hi Ted,
yes these two bugs are fixed. However the missing event would be still very useful for Firebug net's panel. Currently there is no way to find out that a file came directly from FF cache (without any network communication). 
There is a big hack in Firebug to work around this problem (cycle with timeout that is waiting till the file is available in the cache).

The good news is that Michal Novotny sent me a patch that should fix this. It defines the new event "http-on-cached-response". I have tested it and it seems to be working fine. 

I think the event should be sent only if *entire* request is mined from the cache. In case of partial cache content, both events "http-on-modify-request" and "http-on-examine-response" should be sent. But correct me if I am wrong.

Michal could you please attach the latest version of the patch?
Thanks!
Honza
(Assignee)

Comment 8

10 years ago
Created attachment 346282 [details] [diff] [review]
patch v1

http-on-cached-response event is sent when whole document is read from cache without communication with the server. It isn't sent when only a part is read from the cache and the rest is downloaded from the server.
Attachment #346282 - Flags: review?(bzbarsky)
So this will dispatch the notification synchronously under AsyncOpen, right?  It'll also dispatch it for cached 302s, etc.  Do we do the latter in the non-cached case?  Can we possibly send the notification async after AsyncOpen has returned, just like we send http-on-examine-response?
(Assignee)

Comment 10

10 years ago
(In reply to comment #9)
> So this will dispatch the notification synchronously under AsyncOpen, right? 

Yes, this is synchronous.

> It'll also dispatch it for cached 302s, etc.  Do we do the latter in the
> non-cached case?

It is dispatched for cached 302 too. And for non-cached 302 is dispatched http-on-examine-response.

> Can we possibly send the notification async after AsyncOpen
> has returned, just like we send http-on-examine-response?

I can send the notification in OnStartRequest() but this method isn't called for cached 302. So I should dispatch some event for cached 302.
> It is dispatched for cached 302 too. And for non-cached 302 is dispatched
> http-on-examine-response.

OK, good.

I do think we should do this async.
(Assignee)

Comment 12

10 years ago
Created attachment 347859 [details] [diff] [review]
patch v2, the notification is async
Attachment #346282 - Attachment is obsolete: true
Attachment #347859 - Flags: review?(bzbarsky)
Attachment #346282 - Flags: review?(bzbarsky)
Comment on attachment 347859 [details] [diff] [review]
patch v2, the notification is async

>+++ b/netwerk/protocol/http/public/nsIHttpProtocolHandler.idl
>+ * The observer of this topic is notified before data are read from the cache.

"is read"

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
>@@ -329,16 +329,19 @@ nsHttpChannel::Connect(PRBool firstTime)
>         if (mCachedContentIsValid) {
>+            if (!mCachedContentIsPartial) {
>+                AsyncCall(&nsHttpChannel::AsyncOnCachedResponse);
>+            }
>             return ReadFromCache();

Hmm.  Is it ok that the notification will happen after the ReadFromCache call returns?  I guess it should happen before whatever OnStartRequest happens here, which is all that we really need?

That said, if ReadFromCache returs failure the behavior here is a bit weird.  Can we handle that case better?
(Reporter)

Comment 14

10 years ago
> I guess it should happen before whatever OnStartRequest happens here,
> which is all that we really need?
Yes, the notification must be sent before any data are read from cache. So, it's possible to register a listener into nsITraceableChannel and intercept all response data. 

I have just tried patch v2 and all work as expected (I can register nsITraceableChannel and monitor all data). I guess it's because the ReadFromCache is also async, right?

One question: is it possible to ensure this new event (i.e. that response comes directly from FF cache without asking the server)? I need to write an (javascript) unit-test for Firebug to test this issue.

Honza
ReadFromCache only set up other async things, yes.  The events for those are posted from ReadFromCache, so we're saved by event-posting order.

I'm not sure what the last part of comment 14 is asking.
(Reporter)

Comment 16

10 years ago
> I'm not sure what the last part of comment 14 is asking.
I need to write an unit-test for Firebug to test proper behavior when http-on-cached-response event is fired. In order to do it, I need to somehow force Firefox to load a test page directly from the cache without asking the server. How should I do it?
Load it once normally, then load it again?  I believe we have tests that do this already, to test caching...  You could pass LOAD_FROM_CACHE|LOAD_ONLY_FROM_CACHE the second time to be sure.
Comment on attachment 347859 [details] [diff] [review]
patch v2, the notification is async

r=bzbarsky with nit from comment 13 fixed.
Attachment #347859 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

10 years ago
(In reply to comment #15)
> ReadFromCache only set up other async things, yes.  The events for those are
> posted from ReadFromCache, so we're saved by event-posting order.

Yes, it should be OK until something is changes in ReadFromCache().

(In reply to comment #13)
> That said, if ReadFromCache returs failure the behavior here is a bit weird. 
> Can we handle that case better?

What is the problem? That the listener gets notification but no data is actually read because of the error?
Yeah.  There is a notification but the channel doesn't actually even get opened (as in, AsyncOpen throws).

Ideally we'd cancel the runnable we just posted if ReadFromCache fails, basically.
(Assignee)

Comment 21

10 years ago
Created attachment 352174 [details] [diff] [review]
patch v3, event is revoked if ReadFromCache fails
Attachment #347859 - Attachment is obsolete: true
Attachment #352174 - Flags: review?(bzbarsky)
Comment on attachment 352174 [details] [diff] [review]
patch v3, event is revoked if ReadFromCache fails

That looks like it doesn't hold a strong ref to the channel in the event.  That's bad.

Why not just make your event class a subclass of nsRunnableMethod<nsHttpChannel> and override Run() to check the channel before calling the superclass run?
Attachment #352174 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 23

10 years ago
Created attachment 352320 [details] [diff] [review]
patch v4

Actually whole AsyncEvent thing isn't necessary since nsRunnableMethod has already this functionality.
Attachment #352174 - Attachment is obsolete: true
Attachment #352320 - Flags: review?(bzbarsky)
Attachment #352320 - Flags: review?(bzbarsky) → review+
Comment on attachment 352320 [details] [diff] [review]
patch v4

Document that the out param is only set on success and is not refcounted, and looks good.

Comment 25

10 years ago
We should be able to get this in 1.9.2.
Flags: blocking1.9.2?
(Assignee)

Comment 26

10 years ago
Created attachment 353039 [details] [diff] [review]
patch v5
Attachment #352320 - Attachment is obsolete: true
Attachment #353039 - Flags: superreview?(cbiesinger)
Flags: wanted1.9.0.x? → wanted1.9.0.x+
(Reporter)

Comment 27

10 years ago
Patch v5 tested and looks fine. 
Honza

Comment 28

9 years ago
I am confused by the flags set on this bug. I guess Ted said "no" to 1.9.1 but yes to wanted1.9.1, which doesn't make sense to me because I think the work is done here.
John: "blocking1.9.1" just means "would we hold the release of Firefox 3.1 for this bug". They're a way of gauging our progress on the release. Theoretically, if we fixed all the blockers we could release. "wanted1.9.1" just means that we don't think the bug is quite important enough to block the release, but that we would like to get it fixed for that release, and would take a patch. It's generally unrelated to the actual work that is or isn't done on the bug.

Comment 30

9 years ago
Thanks but now I am more confused since this bug has a patch, which as I understand it has been reviewed, revised, tested and is ready to go. So we don't need wanted1.9.1 and if we had it before we should be in 1.9.1 now.
"Patch v5" has an outstanding superreview request. I presume that needs to be finalized before this can land.
Yep.  If you want this in, please mail biesi and ask him to do the sr.
mailing biesi...
Comment on attachment 353039 [details] [diff] [review]
patch v5

+#define NS_HTTP_ON_CACHED_RESPONSE_TOPIC "http-on-cached-response"

can you name this http-on-examine-cached-response? (similar for the function names & the #define name)

+nsHttpChannel::AsyncCall(nsAsyncCallback funcPtr, nsRunnableMethod<nsHttpChannel> **retval)

80 characters per line please
Attachment #353039 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 35

9 years ago
Created attachment 361191 [details] [diff] [review]
patch v6
[Checkin: Comment 38]
Attachment #353039 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Reporter)

Updated

9 years ago
Attachment #361191 - Flags: superreview?(cbiesinger)
(Reporter)

Comment 36

9 years ago
I have asked biesi for patch v6 superreview, but he gave already + for patch v5, so perhaps it's ready for commit?

Honza
(Assignee)

Comment 37

9 years ago
Comment on attachment 361191 [details] [diff] [review]
patch v6
[Checkin: Comment 38]

(In reply to comment #36)
> I have asked biesi for patch v6 superreview, but he gave already + for patch
> v5, so perhaps it's ready for commit?
> 
> Honza

Yes, no other review is needed...
Attachment #361191 - Flags: superreview?(cbiesinger)
Comment on attachment 361191 [details] [diff] [review]
patch v6
[Checkin: Comment 38]


http://hg.mozilla.org/mozilla-central/rev/a2ac3ff41848
Attachment #361191 - Attachment description: patch v6 → patch v6 [Checkin: Comment 38]
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Keywords: checkin-needed
Whiteboard: [firebug-p2] → [firebug-p2] [needs branch landing]
Comment on attachment 361191 [details] [diff] [review]
patch v6
[Checkin: Comment 38]

a191=beltzner
Attachment #361191 - Flags: approval1.9.1+
Landed on the 1.9.1 branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/01c2fd0b1d0d
Keywords: checkin-needed → fixed1.9.1
We're currently in the unfortunate state of having a serious bug in Firefox' net and script panels related to work-around code Firebug has to try to get around this very issue.

Landing this patch will allow us to remove our icky work-around code and rely on this much-nicer solution.

I missed this for 3.0.7 but would love to have in 3.0.8. Conferring with beltzner and Mossop in #developers for branch approval.
Flags: blocking1.9.0.8?
Whiteboard: [firebug-p2] [needs branch landing] → [firebug-p2]
Rob: I think you meant to request approval on the patch instead of blocking. We likely wouldn't _block_ on this bug (speaking without asking others), but we might consider approving the patch.

That being said, I don't see any tests for this bug. Can we get an automated testcase made and checked in? We'll also want this to bake on 1.9.1 a bit before we take it on the stable branch (maybe a week, which means it can still make 1.9.0.8).
Flags: in-testsuite?
I'll be honest: this seems like odd fare for a security and stability release.  It's a nice feature, I agree.  But 1.9.0.x is not about picking up trunk featuers.
(edit of comment #41, I meant "bug in _Firebug's_ net and script panels". Probably obvious to most readers here, but the glitch was bugging me)

Maybe I'm approaching this bug (in Firebug) from the wrong direction. Boris, you're right saying that we shouldn't be pulling new features back from trunk into branch. Can you think of any reason that our net cache interactions would change from Firefox 3.0.6 to 3.0.7?

see issue 1565 for (an attempt at) a description of the problem.

http://code.google.com/p/fbug/issues/detail?id=1565
thanks for the help.
Blocks: 453978
This is not ever going to be a 1.9.0.x blocker. We might look at an approval request for a 1.9.0-branch patch that includes an automated regression testcase. It sounds like something was made worse by some regression, and we'd rather fix that regression than add a new feature as a work-around.
Flags: blocking1.9.0.8? → blocking1.9.0.8-
I accidentally let this get through without tests. Filed bug 482601 to track that. Michal, please don't let me down and get some tests written.
Blocks: 482601
Pushed test to m-c as http://hg.mozilla.org/mozilla-central/rev/0baf72f3b278
Flags: in-testsuite? → in-testsuite+

Comment 50

9 years ago
The regression that caused the problem is being tracking at Bug 482293.

Assuming this landed before FF3.1b3 then we may have a separate problem, since Firebug 1.4a12 on FF3.1b3 show a similar issue as 1.3.3 on FF3.0.7. See the duplicates marked onto 482293. (Its also possible that Firebug code needs to be changed to leverage this fix. Honza?)
Flags: blocking1.9.2?

Comment 51

9 years ago
Sorry, but as usually I cannot decipher the markings on this bug. fixed1.9.1 and target milestone 1.9.2a1, but I want to know: is this fix is in FF3.1b3 where we are testing?
(In reply to comment #51)
> Sorry, but as usually I cannot decipher the markings on this bug. fixed1.9.1
> and target milestone 1.9.2a1, but I want to know: is this fix is in FF3.1b3
> where we are testing?

No
adding this back to the blocking list. we have testcases and approval, and fixed on 1.9.1. This will help us get rid of some extraneous caching in firebug.
Flags: blocking1.9.1- → blocking1.9.1?
If this is fixed on 1.9.1, what do you need the blocking nom for?

Comment 55

9 years ago
Assuming this will come out with FF3.5, we would like to test this feature with Firebug. Is there a build of Firefox available that contains it or a timeline for a release build containing it?
Flags: blocking1.9.1?
(In reply to comment #55)
> Assuming this will come out with FF3.5, we would like to test this feature with
> Firebug. Is there a build of Firefox available that contains it or a timeline
> for a release build containing it?

The nightly builds should have the fix: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.1/
I hope not. Try backing it out and see if the regression goes away?
(Assignee)

Comment 59

9 years ago
I don't think it was caused by this bug. There is no listener for the new topic so the only overhead in Ts test is one call to nsIObserverService::NotifyObservers().
You need to log in before you can comment on or make changes to this bug.