1205410 - Do not wrap channels in Firefox 41

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1120487_do_not_wrap_channels_in_41.patch

Instead of wrapping channels if addons do not implement NewChannel2 we should just fall back to using NewChannel and should *not* wrap the channels for Firefox 41.

Jonas explains the problem in detail:
so here's what happens
10:59 AM <sicking> some piece of gecko calls NS_NewChannel
11:00 AM <sicking> we find a protocol-handler which doesn't implement newChannel2
11:00 AM <sicking> so we wrap the channel with a secchannelwrapper
11:00 AM <sicking> then the gecko code calls channel->asyncOpen(listener)
11:01 AM <sicking> which means that the secwrapchannel calls innerChannel->asyncOpen(listener)
11:01 AM <sicking> at some later point the inner channel starts getting some data
11:02 AM <sicking> so it calls listener->OnStartRequest(this)
11:02 AM <sicking> the problem is that the 'this' is the innerChannel
11:02 AM <sicking> which is not the channel that the gecko code was expecting to get. The gecko code expected to get the secwrapchannel as first argument to OnStartRequest
11:03 AM <ckerschb> I see the problem
11:03 AM <sicking> a lot of gecko code will blow up if it gets an unknown object as first argument to OnStartRequest/OnStopRequest/OnDataAvailable
11:04 AM <sicking> so we need the secwrapchannel to create a new listener and pass that to innerChannel->asyncOpen
11:05 AM <sicking> then when OnStartRequest is called on that listener, we call listener->OnStartRequest(secwrapchannel) on the real listener
11:07 AM <sicking> we might want to back out the secwrapchannel from 41 if possible?
11:09 AM <ckerschb> so, that also landed in 41 (I thought it landed before that) - http://hg.mozilla.org/mozilla-central/rev/4819768c871f


Dan Veditz helped me find addons on AMO that implement their own protocolhandler (so far we found 95 that are on AMO):
https://mxr.mozilla.org/addons/search?string=network%2Fprotocol%3B1%3Fname&find=.manifest&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Liz, Lawrence what do we need to do to make that happen?

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

ni Ritu, who is managing 41.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Liz, Lawrence what do we need to do to make that happen?

Lawrence just told me I should bring that to the attention of Ritu and Sylvestre.

Comment 5

[(Away)]

Christoph, could you please fill out the form for approval-mozilla-release?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Just chatted with ritu on IRC and I was asked to explain the problem in more detail:

If addons use their own implementation of nsIProtocolhandler but do not provide an implementation for newChannel2 [1], then currently we would wrap that channel [2].

For Firefox 41 we shouldn't wrap that channel because as explained in Comment 1, Firefox will crash within OnStartRequest/OnStopRequest/OnDataAvailable if those implementations get an unknown object as the first argument.

To sum it up, what users will be affected:
Users that use addons which use their own implementation of nsIProtocolHandler and do not provide an implementation for newChannel2.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIProtocolHandler.idl#97
[2] https://hg.mozilla.org/mozilla-central/rev/4819768c871f#l1.35

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8662007 [details] [diff] [review]
bug_1120487_do_not_wrap_channels_in_41.patch

Approval Request Comment

[Feature/regressing bug #]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1120487

[User impact if declined]:
User which use addons where those addons use their own implementation of nsIProtocolHandler but do not implement newChannel2 (so far we found 95 such candidates on AMO).

[Describe test coverage new/current, TreeHerder]:
No coverage on treeherder.

[Risks and why]: 
Risks for regular Firefox users are basically zero. Only addon users are affected.

[String/UUID change made/needed]:
no

Comment 8

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

Jorge, Philipp: Is there a way to test this patch quickly that it fixes the issues that showed up as startup crashes in bug 1204324? 

DMajor mentioned that we blocked those two addons, so can we unblock those two addons and then give this patch a test to ensure the crashes are gone? Many Thanks!

Comment 9

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

It's not necessary to unblock the add-ons in order to test this. The add-ons are soft-blocked, meaning you can choose not to disable them, or you can enable them again in the Add-ons Manager.

Comment 10

[[:philipp]]

it is fairly easy to reproduce the crash (or verify that it's fixed in a build). install one of these addons & visit amazon.de - affected builds just crash there after after a few seconds:
https://www.getcocoon.com/downloads/vwc_cocoon.xpi
http://addons.computerbild.de/antiabzocke/plugin/COMPUTERBILD-Abzockschutz-1.0.59.xpi

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66d25d4e4df0

Comment 12

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

Tracked for the remainder of the 41 cycle to ensure we are aware of any regressions, etc.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hrm, testing the linux build from that try push, trying to load amazon.de with cocoon installed just logs https://pastebin.mozilla.org/8846635 and does nothing.

Same with the other extension installed.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Same error logged to the console with Cocoon installed on Windows 10 x64. :\

Comment 15

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

Philipp mentioned the same crash also repros via these STR:

1. Install geolocator addon https://addons.mozilla.org/firefox/addon/geolocater/ 
2. Click on "your location" 
3. Click on "Share your location".

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Ritu Kothari (:ritu) from comment #15)
> Philipp mentioned the same crash also repros via these STR:
> 
> 1. Install geolocator addon
> https://addons.mozilla.org/firefox/addon/geolocater/ 
> 2. Click on "your location" 
> 3. Click on "Share your location".

I can reproduce the crash in 41b9, but with the try build, no crash (but geolocation fails).

Comment 17

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

Here is my experience with the try build. 

1. There was no startup crash after installing cocoon add-on with Try build unlike with 41 RC1.
2. After cocoon add-on is installed on try build and you restart firefox, restore session, none of the tabs have any content.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1204648_update_wrapper_for_41.patch

Jonas, the first patch wasn't quite right, we where still returning an error because of
> if (aLoadInfo != loadInfo) {

Initially we had newChannel2Succeeded as an indicator [1], so I brought that back which seems the right fix now. I tested the addons mentioned in comment 10 and they work fine now.

I do get a warning which is related, but I think I suppose it's safe to ignore for this release:
> [23209] WARNING: no triggering principal available via loadInfo, assuming load is cross-origin: file /home/ckerschb/moz/beta/netwerk/protocol/http/HttpBaseChannel.cpp, line 1147

[1] http://hg.mozilla.org/mozilla-central/diff/4819768c871f/netwerk/base/nsIOService.cpp

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

I pushed the new patch to try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b094c232cd8

Philipp or Wes, in case you get a chance to look at that earlier than I can, please do.
Thanks everyone for your help!

Comment 20

[[:philipp]]

thanks christoph, the try build with this new patch looks good on windows!
i did not observe any crashes (bug 1204324) with the described STR with either computerbild abzockschutz, geolocator or cocoon anymore. 
the addons continue to work normally as well as far as i can tell: you get a successful geolocation with geolocater, and the computerbild one is redirecting you to a warning page if you try to visit one of the domains they deem harmful (they provide a list at http://www.computerbild.de/internet-abzocke/).

Comment 21

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

Christoph, could you please nominate the updated (v2) patch for uplift to mozilla-release? And cancel the uplift request on the patch that had issues. We will most likely take v2 patch in RC3.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8662113 [details] [diff] [review]
bug_1204648_update_wrapper_for_41.patch

Approval Request Comment

[Feature/regressing bug #]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1120487

[User impact if declined]:
User which use addons where those addons use their own implementation of nsIProtocolHandler but do not implement newChannel2 (so far we found 95 such candidates on AMO).

[Describe test coverage new/current, TreeHerder]:
No coverage on treeherder.

[Risks and why]: 
Risks for regular Firefox users are basically zero. Only addon users are affected.

[String/UUID change made/needed]:
no

Comment 23

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

Comment on attachment 8662113 [details] [diff] [review]
bug_1204648_update_wrapper_for_41.patch

As mentioned in the bug description there are ~90 addons that are hosted on AMO that do not implement NewChannel2, and without this fix, when those addons trigger their protocol handlers, they will crash. This makes the issue significant enough to take a fix in 41 RC3. Let's uplift to mozilla-release.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Landed this as https://hg.mozilla.org/releases/mozilla-release/rev/3d007874b9c2

The patch as attached has the wrong bug number, for the record.

Comment 25

[Ada [:adalucinet]]

No crash encountered with steps from comment 10 and comment 15 during our Firefox 41 RC build 3 smoke testing, across platforms [1]. An add-on compatibility spotcheck was also performed with no new issues found.

[1] Windows 7 x64, Windows 10 x86, Mac OS X 10.10.4 and Ubuntu 12.04 x86

Comment 26

[Ioana Chiorean]

On Mobile side we an not reproduce it ob Beta 10 due to addons - they are not mobile compatible. also looking on the signature 0 I can only see Windows OS -https://crash-stats.mozilla.com/report/list?signature=nsXMLHttpRequest%3A%3ASend%28nsIVariant*%2C+mozilla%3A%3Adom%3A%3ANullable%3CT%3E+const%26%29&range_value=7&range_unit=days&date=2015-09-18

Clear on mobile side too.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Just chattet with Ritu on IRC - we can mark this as resolved for 41.

Comment 28

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

This was verified on 41 and therefore reflecting the status as such.