460335 - disallow redirects when updating an application cache

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P3)

Description

[Dave Camp (:dcamp)]

The application cache spec has changed to completely disallow redirects during a cache update (primarily to avoid problems with captive portals).

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

Disabling redirects, make them fatal for manifest and explicit/fallback entries. Includes tests for non-fatal and fatal entry redirects during update and fatal redirect during fetching manifest itself.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v2

As discussed with dcamp on IRC added check to the channel's status to catch redirects and also any call to channel's Cancel() with failure status (e.g. a user call). This removes need for mRedirected member. Tests have better names.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v3

After IRC discussion with dave camp:
- disallowing redirect only when it is not an internal redirect (e.g. proxy replace); redirect abort implemented as cancel of the original channel (as in the previous patch)
- when update is canceled by user we catch it now (indicated by httpChannel->GetStatus(&status), NS_FAILED(status)); there is no worry about offline cache entry used as a main entry, we open it just for READ access
- now network errors are handled correctly

We have to file bugs for handling redirect when normally fetching using an offline cache (if not already fixed) and when loading master entries.

Comment 4

[Dave Camp (:dcamp)]

Comment on attachment 346491 [details] [diff] [review]
v3


>diff --git a/uriloader/prefetch/nsOfflineCacheUpdate.cpp b/uriloader/prefetch/nsOfflineCacheUpdate.cpp
>--- a/uriloader/prefetch/nsOfflineCacheUpdate.cpp
>+++ b/uriloader/prefetch/nsOfflineCacheUpdate.cpp

>     }
>+    NS_ENSURE_SUCCESS(rv, rv);
> 
>+    // Server responded with an error state, let the upper level know about it.
>+    *aStatus = PRUint16(httpStatus);
>+    if (httpStatus >= 400) {
>+        return NS_OK;
>+    }

You might want to use GetRequestSucceeded() here, and maybe include a comment explaining that we need to do this before httpChannel->GetStatus() because we want a server error code to be reported to the caller even if we canceled the channel.

Comment 5

[Dave Camp (:dcamp)]

(In reply to comment #4)

> >+    // Server responded with an error state, let the upper level know about it.
> >+    *aStatus = PRUint16(httpStatus);
> >+    if (httpStatus >= 400) {
> >+        return NS_OK;
> >+    }
> 
> You might want to use GetRequestSucceeded() here, and maybe include a comment
> explaining that we need to do this before httpChannel->GetStatus() because we
> want a server error code to be reported to the caller even if we canceled the
> channel.

Honza pointed out that 3xx codes will be canceled, and we want to treat those as an error.  So the code in the patch be fine.  We need some clear documentation here, since this is kind of strange.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v3.1 [Checkin: Comment 11]

There are more changes, so rerequsting review.

Comment 7

[Dave Camp (:dcamp)]

Comment on attachment 356115 [details] [diff] [review]
v3.1
[Checkin: Comment 11]

Yeah, I like this one better.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 356115 [details] [diff] [review]
v3.1
[Checkin: Comment 11]

You didn't add a comment explaining why you call GetRequestSucceeded before GetStatus, can you do that?

-        if (mState >= nsIDOMLoadStatus::RECEIVING) {
-            *aStatus = NS_ERROR_NOT_AVAILABLE;
-            return NS_OK;
-        }

Why did you remove this code?

Comment 9

[Honza Bambas (:mayhemer)]

That snippet was mirroring xmlhttprequest behavior but xmlhttprequets has changed to return just 0 here and we needed to change it also for internal logic of the class.

Comment 10

[Honza Bambas (:mayhemer)]

Landed on moz-central as ab54983d6ad5.

Comment 11

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8e8c7450183e

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> Landed on moz-central as ab54983d6ad5.

This never happened, afaict.

Comment 13

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/mozilla-central/rev/413fc69d3b3d