missing OnStartRequest when MIME type check and XCTO check failed on a HTTP channel

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: HTTP
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: schien, Assigned: schien)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 8867602 [details] [diff] [review]
ensure-on-start-before-on-stop.patch

Bug 1288361 introduced the MIME type check and X-Content-Type-Options check.
OnStartRequest of the HTTP channel listener will not be called if any of these security check failed.

This breaks the nsIRequestObserver contract because both OnStartRequest and OnStopRequest needs to be called if AsyncOpen failed asynchronously.

STR:
1. create debug build with attachment for additional assertion on the missing OnStartRequest
2. ./mach mochitest dom/security/test/general/test_block_script_wrong_mime.html

Will hit the assertion that shows no OnStartRequest before calling OnStopRequest
Whiteboard: [necko-active]
No longer blocks: 1015466
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8869944 [details]
Bug 1364812 - ensure OnStartRequest is called after nsHttpChannel::CallOnStartRequest. .

https://reviewboard.mozilla.org/r/141510/#review145024

::: netwerk/protocol/http/nsHttpChannel.cpp:1416
(Diff revision 1)
> +        LOG(("  calling mListener->OnStartRequest by ScopeExit [this=%p, "
> +             "listener=%p]\n", this, mListener.get()));
> +        MOZ_ASSERT(!mOnStartRequestCalled);
> +
> +        if (mListener) {
> +            nsCOMPtr<nsIStreamListener> deleteProtector(mListener);

Do you know why we need this? I just want to understand.
Attachment #8869944 - Flags: review?(dd.mozilla) → review+
(Assignee)

Comment 3

a year ago
mozreview-review
Comment on attachment 8869944 [details]
Bug 1364812 - ensure OnStartRequest is called after nsHttpChannel::CallOnStartRequest. .

https://reviewboard.mozilla.org/r/141510/#review145088

::: netwerk/protocol/http/nsHttpChannel.cpp:1416
(Diff revision 1)
> +        LOG(("  calling mListener->OnStartRequest by ScopeExit [this=%p, "
> +             "listener=%p]\n", this, mListener.get()));
> +        MOZ_ASSERT(!mOnStartRequestCalled);
> +
> +        if (mListener) {
> +            nsCOMPtr<nsIStreamListener> deleteProtector(mListener);

The original code from line 1502 is introduced by bug 1328121. Usually this kind of pattern is to avoid nsHttpChannel object to release the reference to mListener during mListener::OnStartRequest call stack, probably cause by calling nsIChannel::Cancel. With the deleteProtector we can ensure the listener object will not be destructed before OnStartRequest is returned.
Attachment #8867602 - Attachment is obsolete: true

Comment 4

a year ago
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90d9d225e54f
ensure OnStartRequest is called after nsHttpChannel::CallOnStartRequest. r=dragana.

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/90d9d225e54f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.