Support AsyncOpen2,Open2,nsIUploadChannel and nsIUploadChannel2 on nsSecCheckWrapChannelBase

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41- wontfix, firefox42 fixed, firefox43 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
(Assignee)

Updated

2 years ago
Blocks: 1143922
(Assignee)

Updated

2 years ago
Blocks: 1204324
(Assignee)

Comment 1

2 years ago
Created attachment 8660918 [details] [diff] [review]
bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

I think that should do it - probably we even want to uplift that patch since we landed Bug 1143922 within 42.
Attachment #8660918 - Flags: review?(jonas)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8660918 [details] [diff] [review]
bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8660918 [details] [diff] [review]
> bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch
> 
> I think that should do it - probably we even want to uplift that patch since
> we landed Bug 1143922 within 42.

Dang - we should also forward 'nsIUploadChannel'
Attachment #8660918 - Flags: review?(jonas)
(Assignee)

Comment 3

2 years ago
Created attachment 8660922 [details] [diff] [review]
bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

That should do it.
Attachment #8660918 - Attachment is obsolete: true
Attachment #8660922 - Flags: review?(jonas)
Attachment #8660922 - Flags: review?(jonas) → review-
(Assignee)

Comment 4

2 years ago
Created attachment 8660926 [details] [diff] [review]
bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

As Jonas pointed out we should forward calls for nsIUploadChannel as well as nsIUploadChannel2.
Attachment #8660922 - Attachment is obsolete: true
Attachment #8660926 - Flags: review?(jonas)
Comment on attachment 8660926 [details] [diff] [review]
bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

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

Looks good, though the patch description is no longer quite accurate.
Attachment #8660926 - Flags: review?(jonas) → review+
(Assignee)

Updated

2 years ago
Summary: Implement AsyncOpen2() on nsSecCheckWrapChannelBase → Support AsyncOpen2,Open2,nsIUploadChannel and nsIUploadChannel2 on nsSecCheckWrapChannelBase
(Assignee)

Comment 6

2 years ago
Created attachment 8660940 [details] [diff] [review]
bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

Updated bug as well as patch description. Carrying over r+ from Jonas.
Attachment #8660926 - Attachment is obsolete: true
Attachment #8660940 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6a370182bf
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6f6a370182bf
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 9

2 years ago
Comment on attachment 8660940 [details] [diff] [review]
bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

Approval Request Comment

[Feature/regressing bug #]:
Bug 1143922 - Add AsyncOpen2 to nsIChannel and perform security checks when opening a channel

[User impact if declined]:
Breaks addons that haven't updated their code to use newChannel2/asyncOpen2.

[Describe test coverage new/current, TreeHerder]:
No automated tests, only manual testing, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1204324#c27

[Risks and why]:
low, because only older addons are affected by the change.

[String/UUID change made/needed]:
no
Attachment #8660940 - Flags: approval-mozilla-aurora?
Dave, Jorge: Could you please weigh in on the risk associated with taking this patch in 41 RC2? DMajor mentioned that this is the right fix for the addons that we had to disable due to crashes in bug 1204324. Philipp's testing has also verified the fix, see https://bugzilla.mozilla.org/show_bug.cgi?id=1204324#c27 

If this fix is safe, it definitely seems like the right thing to do to support our addons community.
Flags: needinfo?(jorge)
Flags: needinfo?(dtownsend)
Tracking for 41 as this might be included in 41 RC2.
status-firefox41: --- → affected
tracking-firefox41: --- → +
That looks okay in terms of compatibility. However, I'm unclear on what this means for the add-ons that we blocked (and others that are affected). Will they work correctly after this fix, and should I unblock them if this lands in 41? Is this a temporary fix?
Flags: needinfo?(jorge)
(In reply to Ritu Kothari (:ritu) from comment #10)
> Dave, Jorge: Could you please weigh in on the risk associated with taking
> this patch in 41 RC2?

I don't really understand what this patch is doing so I can't comment on the risk of taking it.
Flags: needinfo?(dtownsend)
It's not a temporary fix. It's the correct one.

Philip said that the addons got fixed. That's all the data I have to go on. Based on the crash stack it makes sense that the problem was fixed, but what I *think* the addon is doing is a little bit crazy so it's unclear if all problems have been fixed.
And to be clear, I think it's pretty safe to take the patch (probably more safe than not taking it). What I'm less sure about is if we should unblock the addons or not.
status-firefox42: --- → affected
Comment on attachment 8660940 [details] [diff] [review]
bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

We have quite some time to find potential regressions in 42, taking it.
Attachment #8660940 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7677798031de
status-firefox42: affected → fixed
(Assignee)

Comment 18

2 years ago
Created attachment 8661947 [details] [diff] [review]
bug_1204648_update_wrapper_for_41.patch

I just realized that the problem is most likely not asyncOpen2() in 41, but rather that we are not forwaring nsIUploadChannel and nsIUploadChannel2. Jonas, if you agree, that would could uplift that patch to 41 to get a fix for addons.
Flags: needinfo?(jonas)
Untracking and marking as wontfix for 41. It is too late as I gtb RC2 in an hour or so. For 41, we may just need to live with blocking a few addons, so far only 2.
status-firefox41: affected → wontfix
tracking-firefox41: + → -
Yes, the problem is not AsyncOpen2, but rather the wrapping channel.
Flags: needinfo?(jonas)
You need to log in before you can comment on or make changes to this bug.