1153256 - [e10s] with NoScript, crash in mozilla::net::HttpChannelParentListener::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned __int64, unsigned int)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Tracy Walker [:tracy]]

This bug was filed from the Socorro interface and is 
report bp-ee324363-03db-4d17-a6b2-61f672150408.
=============================================================

This was last fixed in bug 1106396 in earlier Feb. 2015. That eliminated most of the crashes. After that fix landing we were getting 0-5 crash reports a day.  Up until recently, on 2015040303, when the volume elevated to around 40 crashes per day. Bringing it up to #2 topcrash in volume on Nightly.

Comment 1

[Dragana Damjanovic [:dragana]]

There is a problem with NoScript addon that does not do redirect properly and i think all of this reports have this addon.

Comment 2

[Patrick McManus [:mcmanus]]

honza, do you think you could sort this out?

It seems to be releated to https://bugzilla.mozilla.org/show_bug.cgi?id=975338 which was jason and steve..

Comment 3

[Dragana Damjanovic [:dragana]]

I have seen this in original bug 1106396
Explanation what is happening:
https://bugzilla.mozilla.org/show_bug.cgi?id=1106396#c30

I think it was not fix in version 2.6.9, as I wrote in the comment.

Comment 4

[Patrick McManus [:mcmanus]]

can we make core-gecko at least not crash? I tihnk its ok if the old add-on just doesn't work.

Comment 5

[Dragana Damjanovic [:dragana]]

I could fix this as soon as i am back in the office, on Monday.

Comment 6

[Dragana Damjanovic [:dragana]]

Explanation what is happening: It is breaking because a channel is not suspended for diversion and it should be, but the real problem starts earlier when redirect is not finished properly. OnRedirectResult is not call and in this function the HttpChannelParent replaces the old httpChannel with the new one. So if OnRedirectResult is not called, during the diversion HttpChannelParent suspend the wrong channel.


It would be possible to recognize this case: OnRedirectResult is not called but OnStartRequest from a new channel is called. It would be possible to call OnRedirectResult before calling OnStartRequest for listeners, but it will be a hack. The whole redirect code is complex enough without this and this hack will mask other future bugs (there is one that I know not really important one I forgot the bug number). 

So my question is how important is to make gecko not crash for NoScript?

Comment 7

[Patrick McManus [:mcmanus]]

hmm.

maybe the blocklist should be used for old revisions of the add-on. ni jorge.

The real tricky bit might be because of e10s though, right? This is tied more to e10s than gecko version number.. iirc the blocklist can handle gecko version numbers, but I don't know if it can become dependent on the e10s pref.. and last I knew e10s wasn't expected to move forward with the rest of the train yet.

Comment 8

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

Is this crash happening with specific versions of the add-on, specific versions of Nightly, a combination of both?

Comment 9

[Patrick McManus [:mcmanus]]

Thanks Jorge,

my understanding, from comment 3, is that versions of the noscript addon before 2.6.9 (esp 2.6.6.) have this problem.. and it turns into a crash when interacting with any e10s gecko. Right now e10s is only nightly-40 right? But I believe that particular feature isn't tied to the release trains in the usual way.

So we need people using an old rev of the addon to update the addon (or not use it) if they're using e10s.

As Dragana points in comment 6 we probably can accommodate the broken user by adding some significant cruft to an already rickety piece of gecko.. and an addon as big as noscript might merit that in general - but in this case what we really want to do is trigger the upgrade to where the addon is already fixed.

Comment 10

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

It looks like the majority of NoScript users are on 2.6.9 and above, so I could flag lower versions as incompatible with Firefox 40 and above. That should encourage people to update.

Thoughts? Giorgio?

Comment 11

[Giorgio Maone [:ma1]]

(In reply to Jorge Villalobos [:jorgev] from comment #10)
> It looks like the majority of NoScript users are on 2.6.9 and above, so I
> could flag lower versions as incompatible with Firefox 40 and above. That
> should encourage people to update.
> 
> Thoughts? Giorgio?

Please proceed. 
I'd do it myself, but I suppose it's easier for you to bulk-update the version info for all the versions <= 2.6.9.

Comment 12

[Dragana Damjanovic [:dragana]]

Sorry for not checking this earlier.
I thought they have fix this but i have seen crash reports with version 2.6.9.22 which is the latest one.


I will see to contact someone to get this fixed in the next NoScript version.

Comment 13

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

Dragana, Giorgio is the NoScript dev, so we can have that discussion here.

I added the compat override for the older versions. Are the crashes more prevalent there or is the problem still as bad in the latest versions?

Comment 14

[Giorgio Maone [:ma1]]

(In reply to Dragana Damjanovic [:dragana] from comment #12)

> I will see to contact someone to get this fixed in the next NoScript version.

Dragana, it seems you managed to spot the wrong behavior, didn't you?
If so, could you please explain me what should NoScript chnage exactly to fix this problem?

Comment 15

[Dragana Damjanovic [:dragana]]

(In reply to Giorgio Maone from comment #14)
> (In reply to Dragana Damjanovic [:dragana] from comment #12)
> 
> > I will see to contact someone to get this fixed in the next NoScript version.
> 
> Dragana, it seems you managed to spot the wrong behavior, didn't you?
> If so, could you please explain me what should NoScript chnage exactly to
> fix this problem?

There has been a change in how a redirect is handled internally because of e10s and no e10s do not need this additional step.

I had a look at  NoScript code, but without going into details it is hard to see what is the best solution. So I have one question: can you use nsIHttpChannel.redirectTo, probably not?

If you do not use redirectTo you will need to call onRedirectResult if the next listener is nsIRedirectResultListener
This is necko code:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#212

If redirect was successful onRedirectResult is called right after AsyncOpen of the new channel. So it should be called before onStartRequest of the new channel is called.

Probably in onRedirectVerifyCallback you should call newChan.AsyncOpen and then onRedirectResult(true/false)

Thanks a lot for fixing this.

Comment 17

[Giorgio Maone [:ma1]]

(In reply to Dragana Damjanovic [:dragana] from comment #15)
> So I have one question: can you use
> nsIHttpChannel.redirectTo, probably not?

Unfortunately I could use it only for the functionality which EFF's HTTPS Everywhere add-on was derived from -- in facts, if I recall correctly, redirectTo() was created later mostly for simplifying HTTPS Everywhere's code.

The ChannelReplacement machinery is used elsewhere to suspend a channel before it hits the network with actual HTTP traffic but after DNS resolution is finalized, perform various checks asynchronously on it, then resume it in a possibly modified form. It's a terrible hack, but when I had to figure it out there was no other way to reliably implement a selective CSRF protection in ABE ( https://noscript.net/abe )

> If you do not use redirectTo you will need [...]

Thank you very much for these hints, I'm looking into them and trying to fix this ASAP.

Comment 18

[Giorgio Maone [:ma1]]

Dragana, is there any way to reliably reproduce the crash?
I'm trying to put the onRedirectResult() call in different places (immediately after asyncOpen it seems to prevent the navigation), but when I find the right spot not to break anything I'd like to be sure it also actually fixes this bug.
Thank you!

Comment 19

[Dragana Damjanovic [:dragana]]

This is one reproducable crash, but timing is critical so it can be that it does not crash with each build:
https://bugzilla.mozilla.org/show_bug.cgi?id=1106396#c2

The crash will occur if you redirect a download. If download is larger it is more certain that it will happen. Putting it other way around, if a download is too short it can happen that it does not crash.


(In reply to Giorgio Maone from comment #18)
> Dragana, is there any way to reliably reproduce the crash?
> I'm trying to put the onRedirectResult() call in different places
> (immediately after asyncOpen it seems to prevent the navigation), but when I
> find the right spot not to break anything I'd like to be sure it also
> actually fixes this bug.
> Thank you!

You should be careful how you fix this problem because it can cause more problems. If I understand you correctly you want to put onRedirectResult after asyncOpen, that can cause other problems.

At some point you call onChannelRedirect or asyncOnChannelRedirect. If you call asyncOnChannelRedirect, it can return failure or success. If it returns success the sink MUST call onRedirectVerifyCallback with success of failure code (so the sink can veto a redirect). And therefore you can wait for this call. If it return failure onRedirectVerifyCallback does not have to be called so do not wait for it. When onRedirectVerifyCallback is called with a success you can call asyncOpen and cancel the old channel (the old channel is suspended before calling redirect and when it is cancel it is resumed, that is what necko code do so that it can continue using the old channel if redirect is vetoed, i do not know what you addon needs). Immediately after asyncOpen you need to call onRedirectResult if the next listener is nsIRedirectResultListener

Please look at this spec:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIChannelEventSink.idl#55


sorry if I just misunderstood your comment #18.

Comment 20

[Giorgio Maone [:ma1]]

Dragana, please check 2.6.9.23rc1 from https://noscript.net/getit#devel
Thank you!

Comment 21

[Dragana Damjanovic [:dragana]]

Unfortunately it is not fix. The code looks reasonable, but there are still crashes:
https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=14&signature=mozilla%3A%3Anet%3A%3AHttpChannelParentListener%3A%3AOnDataAvailable%28nsIRequest*%2C+nsISupports*%2C+nsIInputStream*%2C+unsigned+__int64%2C+unsigned+int%29#tab-sigsummary

go to Correlations -> Add-on by version


I have looked again into the comments that some people left and found a way to reproduce it:
Just trying to download a zip or gz file from my google drive causes a crash with NoScript.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Top crash, tracking.

Comment 23

[Giorgio Maone [:ma1]]

(In reply to Dragana Damjanovic [:dragana] from comment #21)
> found a way to reproduce it:
> Just trying to download a zip or gz file from my google drive causes a crash
> with NoScript.

Unfortunately I cannot reproduce yet, but I took the pretty radical path of removing the ChannelReplacement redirection emulation hack in favor of redirectTo(). This approach has got its own share of issues but at least should not crash.

Since it seems you can reproduce, could you please install 2.6.9.26rc1 from https://noscript.net/getit#devel and verify? Thank you!

Comment 24

[Dragana Damjanovic [:dragana]]

(In reply to Giorgio Maone from comment #23)
> (In reply to Dragana Damjanovic [:dragana] from comment #21)
> > found a way to reproduce it:
> > Just trying to download a zip or gz file from my google drive causes a crash
> > with NoScript.
> 
> Unfortunately I cannot reproduce yet, but I took the pretty radical path of
> removing the ChannelReplacement redirection emulation hack in favor of
> redirectTo(). This approach has got its own share of issues but at least
> should not crash.
> 

Thanks a lot for changing this.


> Since it seems you can reproduce, could you please install 2.6.9.26rc1 from
> https://noscript.net/getit#devel and verify? Thank you!


I have tried the new version and I could not reproduce the crash from comment #21.
But let's wait a bit and watch crash statistics.

Comment 25

[Dragana Damjanovic [:dragana]]

It is crashing much less often now, but there are 1-2 crashes with 2.6.9.26rc3

https://crash-stats.mozilla.com/report/index/13fdc64e-e39f-4639-8ac3-412892150608

Comment 26

[Giorgio Maone [:ma1]]

(In reply to Dragana Damjanovic [:dragana] from comment #25)
> It is crashing much less often now, but there are 1-2 crashes with
> 2.6.9.26rc3
> 
> https://crash-stats.mozilla.com/report/index/13fdc64e-e39f-4639-8ac3-
> 412892150608

Do they have channel-fiddling extensions other than NoScript in common (I'm looking at HTTPS Everywhere and donottrackplus, for instance)?

Comment 27

[Dragana Damjanovic [:dragana]]

https://crash-stats.mozilla.com/report/index/f1d9a4c6-a0b5-4dfb-b43b-5b5a72150606
https://crash-stats.mozilla.com/report/index/13fdc64e-e39f-4639-8ac3-412892150608
https://crash-stats.mozilla.com/report/index/fc9e1338-b73f-4979-9f84-a14c32150608
https://crash-stats.mozilla.com/report/index/ab2a3a1f-f0f9-4c6a-8abf-4b0382150608


this are the crashes and all of them have donottrackplus and HTTPS Everywhere. All 4 have the same addons.

Comment 28

[Giorgio Maone [:ma1]]

(In reply to Dragana Damjanovic [:dragana] from comment #27)
> all of them have donottrackplus and HTTPS
> Everywhere. All 4 have the same addons.

I doubt NoScript is the real culprit anymore, then.

Comment 29

[Dragana Damjanovic [:dragana]]

In the last 7 days there was only 8 crashes. 7 with the old version of NoScript and one with an old Firefox build from January there was an error in our code that was fix now.

Should we flag old versions as incompatible? And close this bug

I have not seen the crashes from comment #27 any more, but I will keep observing if they happen again.

Comment 30

[Giorgio Maone [:ma1]]

(In reply to Dragana Damjanovic [:dragana] from comment #29)

> Should we flag old versions as incompatible? And close this bug

Fine with me. 
Any version < 2.6.9.26rc3 can be kept out from Nightly.

Comment 31

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

I updated the override per comment #30.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

No longer tracking for 40. We are going to move this version to the beta channel and e10s won't be available

Comment 33

[Dragana Damjanovic [:dragana]]

Still only crashes with old NoScript version. Closing this bug...

Comment 34

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

Untracked for FF41 given that e10s is not enabled by default. Also, this issue seems fixed on FF42 so we are good.