1204648 - Support AsyncOpen2,Open2,nsIUploadChannel and nsIUploadChannel2 on nsSecCheckWrapChannelBase

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

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'

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

That should do it.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

As Jonas pointed out we should forward calls for nsIUploadChannel as well as nsIUploadChannel2.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1204648_asyncOpen2_nsSecCheckWrapChannelBase.patch

Updated bug as well as patch description. Carrying over r+ from Jonas.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6a370182bf

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f6a370182bf

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracking for 41 as this might be included in 41 RC2.

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

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?

Comment 13

[Dave Townsend [:mossop]]

(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.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7677798031de

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, the problem is not AsyncOpen2, but rather the wrapping channel.