Closed
Bug 449198
Opened 17 years ago
Closed 16 years ago
http-on-examine-response isn't fired when a response comes from the cache
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: Honza, Assigned: michal)
References
Details
(Keywords: fixed1.9.1, Whiteboard: [firebug-p2])
Attachments
(1 file, 5 obsolete files)
6.62 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.
Comment 1•17 years ago
|
||
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•17 years ago
|
||
Yep, separate event would work for me.
Updated•17 years ago
|
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Reporter | ||
Comment 3•17 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
Comment 4•17 years ago
|
||
this would be really useful for firebug going forward. Does anybody have any cycles to take a look at it?
Whiteboard: [firebug-p2]
Updated•17 years ago
|
Flags: blocking1.9.1?
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → michal
Comment 6•17 years ago
|
||
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•17 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•17 years ago
|
||
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)
![]() |
||
Comment 9•17 years ago
|
||
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•17 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.
![]() |
||
Comment 11•17 years ago
|
||
> 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•17 years ago
|
||
Attachment #346282 -
Attachment is obsolete: true
Attachment #347859 -
Flags: review?(bzbarsky)
Attachment #346282 -
Flags: review?(bzbarsky)
![]() |
||
Comment 13•17 years ago
|
||
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•17 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
![]() |
||
Comment 15•17 years ago
|
||
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•17 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?
![]() |
||
Comment 17•17 years ago
|
||
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 18•17 years ago
|
||
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•17 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?
![]() |
||
Comment 20•17 years ago
|
||
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•17 years ago
|
||
Attachment #347859 -
Attachment is obsolete: true
Attachment #352174 -
Flags: review?(bzbarsky)
![]() |
||
Comment 22•17 years ago
|
||
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•17 years ago
|
||
Actually whole AsyncEvent thing isn't necessary since nsRunnableMethod has already this functionality.
Attachment #352174 -
Attachment is obsolete: true
Attachment #352320 -
Flags: review?(bzbarsky)
![]() |
||
Updated•17 years ago
|
Attachment #352320 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 24•17 years ago
|
||
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.
Assignee | ||
Comment 26•17 years ago
|
||
Attachment #352320 -
Attachment is obsolete: true
Attachment #353039 -
Flags: superreview?(cbiesinger)
Updated•17 years ago
|
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Reporter | ||
Comment 27•17 years ago
|
||
Patch v5 tested and looks fine.
Honza
Comment 28•17 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.
Comment 29•17 years ago
|
||
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•17 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.
Comment 31•17 years ago
|
||
"Patch v5" has an outstanding superreview request. I presume that needs to be finalized before this can land.
![]() |
||
Comment 32•17 years ago
|
||
Yep. If you want this in, please mail biesi and ask him to do the sr.
Comment 33•17 years ago
|
||
mailing biesi...
Comment 34•16 years ago
|
||
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
Updated•16 years ago
|
Attachment #353039 -
Flags: superreview?(cbiesinger) → superreview+
Assignee | ||
Comment 35•16 years ago
|
||
Attachment #353039 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•16 years ago
|
Attachment #361191 -
Flags: superreview?(cbiesinger)
Reporter | ||
Comment 36•16 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•16 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 38•16 years ago
|
||
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]
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Updated•16 years ago
|
Keywords: checkin-needed
Whiteboard: [firebug-p2] → [firebug-p2] [needs branch landing]
Comment 39•16 years ago
|
||
Attachment #361191 -
Flags: approval1.9.1+
Comment 40•16 years ago
|
||
Landed on the 1.9.1 branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/01c2fd0b1d0d
Keywords: checkin-needed → fixed1.9.1
Comment 41•16 years ago
|
||
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?
Updated•16 years ago
|
Whiteboard: [firebug-p2] [needs branch landing] → [firebug-p2]
Comment 42•16 years ago
|
||
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?
![]() |
||
Comment 43•16 years ago
|
||
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.
Comment 44•16 years ago
|
||
(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
![]() |
||
Comment 45•16 years ago
|
||
Here's the list of 1.9.0.7 fixes:
https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&long_desc_type=substring&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=anywords&keywords=fixed1.9.0.7+verified1.9.0.7&emailassigned_to1=1&emailtype1=exact&email1=&emailassigned_to2=1&emailreporter2=1&emailqa_contact2=1&emailtype2=exact&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=
None of those jump out at me as cache-related.
If you do have a change in behavior between 1.9.0.6 and 1.9.0.7, I'd suggest following the advice in http://groups.google.com/group/mozilla.dev.apps.firefox/msg/c1e804af76907b5e
Of course that same advice was already given in that same group by that same person in response to a specific question about your problem 3 days ago: http://groups.google.com/group/mozilla.dev.apps.firefox/msg/6073f13836450994
Comment 46•16 years ago
|
||
thanks for the help.
Comment 47•16 years ago
|
||
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-
Comment 48•16 years ago
|
||
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
![]() |
||
Comment 49•16 years ago
|
||
Pushed test to m-c as http://hg.mozilla.org/mozilla-central/rev/0baf72f3b278
Flags: in-testsuite? → in-testsuite+
Comment 50•16 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•16 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?
Comment 52•16 years ago
|
||
(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
Comment 53•16 years ago
|
||
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?
![]() |
||
Comment 54•16 years ago
|
||
If this is fixed on 1.9.1, what do you need the blocking nom for?
Comment 55•16 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?
Updated•16 years ago
|
Flags: blocking1.9.1?
Comment 56•16 years ago
|
||
(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/
Comment 57•16 years ago
|
||
Did this cause a regression on OSX Ts?
http://graphs-new.mozilla.org/#tests=[{%22test%22:%2216%22,%22branch%22:%223%22,%22machine%22:%2236%22}]&sel=1236381876,1236465509
Comment 58•16 years ago
|
||
I hope not. Try backing it out and see if the regression goes away?
Assignee | ||
Comment 59•16 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.
Description
•