Closed Bug 758973 Opened 8 years ago Closed 7 years ago

HTTP status 308 response results in nsIChannelEventSink::REDIRECT_TEMPORARY passed to nsIChannelEventSink instead of nsIChannelEventSink::REDIRECT_PERMANENT

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: briansmith, Assigned: julian.reschke)

References

()

Details

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #714302 +++

See ContinueProcessRedirectionAfterFallback:

    if (mRedirectType == 301) // Moved Permanently
        redirectFlags = nsIChannelEventSink::REDIRECT_PERMANENT;
    else
        redirectFlags = nsIChannelEventSink::REDIRECT_TEMPORARY;

It is a good idea to grep through all the code (especially in netwerk/ and in content/, where XHR lives) to see all the cases where 301 is handled specially.

I know that before we agreed that, as far as Necko is concerned, there should be no difference in the various kinds of redirects, but we should make sure 307/301 are reported to users of Necko correctly so they can make their own decisions as to what distinctions to make between them.

(Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various statuses that are to be considered redirects in some cases, and it omits status 308.)
(BTW, it is confusing that 301 is permanent and 302 is temporary, but 307 is temporary and 308 is permanent.)
Summary: HTTP status 307 response results in nsIChannelEventSink::REDIRECT_TEMPORARY passed to nsIChannelEventSink instead of nsIChannelEventSink::REDIRECT_PERMANENT → HTTP status 308 response results in nsIChannelEventSink::REDIRECT_TEMPORARY passed to nsIChannelEventSink instead of nsIChannelEventSink::REDIRECT_PERMANENT
(In reply to Brian Smith (:bsmith) from comment #0)
> +++ This bug was initially created as a clone of Bug #714302 +++
> 
> See ContinueProcessRedirectionAfterFallback:
> 
>     if (mRedirectType == 301) // Moved Permanently
>         redirectFlags = nsIChannelEventSink::REDIRECT_PERMANENT;
>     else
>         redirectFlags = nsIChannelEventSink::REDIRECT_TEMPORARY;
> 
> It is a good idea to grep through all the code (especially in netwerk/ and
> in content/, where XHR lives) to see all the cases where 301 is handled
> specially.
> 
> I know that before we agreed that, as far as Necko is concerned, there
> should be no difference in the various kinds of redirects, but we should
> make sure 307/301 are reported to users of Necko correctly so they can make
> their own decisions as to what distinctions to make between them.

Ack. Will check and provide a patch.

> (Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various
> statuses that are to be considered redirects in some cases, and it omits
> status 308.)

Indeed. That's the code where XHR requires redirects to be followed, which is problematic in itself.

Optimally. XHR would (a) allow callers to disable following redirects and (b) actually state what to do with 3xx codes in general.
(In reply to Brian Smith (:bsmith) from comment #0)
> ...
> (Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various
> statuses that are to be considered redirects in some cases, and it omits
> status 308.)

-> <https://www.w3.org/Bugs/Public/show_bug.cgi?id=17222>
Attached patch proposed patch, work-in-progress (obsolete) — Splinter Review
Adds checks for 308 to 2 more places. Added a function for classification in HttpBaseChannel.cpp; but I'm not sure how to use it in nsHttpResponseHead.cpp; maybe it needs to be moved somewhere else. Also, optimally, we had test coverage for this; do we have test cases where the permanence bis is tested somehow?
Attachment #627859 - Flags: review?(bsmith)
Comment on attachment 627859 [details] [diff] [review]
proposed patch, work-in-progress

I recommend that you make IsPermanentRedirect a static member of nsHttpResponseHead so that you can use it in the ComputeFreshnessLife as well as in nsHttpChannel.

The patch looks good to me, but I am going to ask Christian to look at it too in the hope that he might be able to suggest a convenient way to test it.
Attachment #627859 - Flags: review?(bsmith) → review?(cbiesinger)
Comment on attachment 627859 [details] [diff] [review]
proposed patch, work-in-progress

+    if ((mStatus == 300) || (mStatus == 301) || (mStatus == 308)) {

this should probably use the function too. make the function a static member of nsHttp? (in nsHttp.h/cpp)
Attachment #627859 - Flags: review?(cbiesinger) → review-
Attached patch Proposed patch (obsolete) — Splinter Review
Moved as suggested.

I wonder whether we should do the same for IsSafeMethod and ShouldRewriteRedirectToGET (for consistency)?
Attachment #627859 - Attachment is obsolete: true
Attachment #642214 - Flags: review?(bsmith)
Comment on attachment 642214 [details] [diff] [review]
Proposed patch

Review of attachment 642214 [details] [diff] [review]:
-----------------------------------------------------------------

If you want to move the existing methods for consistency then please do so in a separate patch and r+ me ahead of time for it.

::: netwerk/protocol/http/nsHttp.cpp
@@ +290,5 @@
> +bool
> +nsHttp::IsPermanentRedirect(PRUint32 httpStatus)
> +{
> +  return httpStatus == 301 || httpStatus == 308;
> +}   

Trailing whitespace.
Attachment #642214 - Flags: review?(bsmith) → review+
(In reply to Brian Smith (:bsmith) from comment #8)
> If you want to move the existing methods for consistency then please do so
> in a separate patch and r+ me ahead of time for it.

See bug 782235.
Attached patch Proposed patchSplinter Review
Attachment #642214 - Attachment is obsolete: true
Attachment #651325 - Flags: review?(bsmith)
Blocks: 782235
Brian, can I set this to "r+" myself (the only change was the removal of trailing whitespace)?
Comment on attachment 651325 [details] [diff] [review]
Proposed patch

Review of attachment 651325 [details] [diff] [review]:
-----------------------------------------------------------------

When somebody gives you r+ with a request to fix some trivial issue (naming, whitespace, etc.), you can automatically r+ the new version of the patch when you attach it.
Attachment #651325 - Flags: review?(bsmith) → review+
Keywords: checkin-needed
I don't see any Try results here, so I pushed. I'll land it if it goes green.
https://tbpl.mozilla.org/?tree=Try&rev=07e44b46b028
https://hg.mozilla.org/mozilla-central/rev/c4c2b82b79a2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.