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

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

ni Ritu, who is managing 41.

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

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

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

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!

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.

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

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

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.

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

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

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

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.

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

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!

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/).

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

Landed this as https://hg.mozilla.org/releases/mozilla-release/rev/3d007874b9c2 The patch as attached has the wrong bug number, for the record.

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

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.

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

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