Closed Bug 1087674 Opened 10 years ago Closed 10 years ago

Firefox won't close with Avast 2015 10.0.2206 when HTTPS scanning is enabled

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Version:
33 Branch
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
firefox33 + fixed
firefox34 + fixed
firefox35 + fixed
firefox36 + fixed
firefox-esr31 --- unaffected
relnote-firefox --- 33+

People

(Reporter: mvocom, Assigned: Yoric)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 4 obsolete files)

Making GMPInstallManager more resilient to abort(), timeout or certificate errors
14.43 KB, patch
Yoric
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Sylvestre
: approval-mozilla-release+
Details | Diff | Splinter Review
Uplift version
14.36 KB, patch
Yoric
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141011015303

Steps to reproduce:

I updated yesterday to Avast 2015 10.0.2206.
Often (not always), when closing the browser the window is closed but FF is still running.
I am the OP of the Avast Forums thread above.  I also have a thread at http://forums.mozillazine.org/viewtopic.php?f=38&t=2882327
If Firefox crashes when you're tying to close it with Avast installed, could you poste some crash IDs URLs (type about:crashes).
Flags: needinfo?(mvocom)
Severity: normal → critical
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak | nsDebugImpl::Abort(char const*, int) ]
Keywords: crash
I don't know Avast but many AV have web protection feature that cause various issues with browsers.
Could you try to disable the web protection in Avast and test?
I disabled https scanning(a new feature) in Avast WebShield with Firefox closed.  Since then, I've visited https sites and closed Firefox several times, including with a different Windows user and Firefox profile.  There were no crashes after closing, and Firefox closed promptly instead of hanging for a considerable amount of time.

This means it is possible to have the past WebShield protection.  All one loses is the https scanning.  I'll report this to Avast in my thread there, in the hope that they will correct the problem.  I wonder why it doesn't affect Opera.
https://crash-stats.mozilla.com/report/index/877bc118-5009-409f-a807-821572141023

I used (In reply to Loic from comment #5)
> I don't know Avast but many AV have web protection feature that cause
> various issues with browsers.
> Could you try to disable the web protection in Avast and test?

Thanks Loic.

https://crash-stats.mozilla.com/report/index/877bc118-5009-409f-a807-821572141023

I used to terminate FF before the Crash Reporter came up and hence only one.
I'll also try it with "https scanning" disabled and update here.

There are numerous reports in Avast forum about FF crashes.
Do you think the issue should be addressed by FF or Avast? Or both?
Flags: needinfo?(mvocom)
Another one:
https://crash-stats.mozilla.com/report/index/c9adc4b0-1f57-4b10-85ed-443602141023

"Add-on manager -> Check for (installed) updates" consistently crashes FF.
I does not happen with "https scanning" disabled in Avast.
It's not very surprising that the module about scanning SSL traffic is involved, in the past, this module has been the source of various issues.
Thank you. What's next? :)
Contact the support of Avast.
We've done that. Thanks.
(In reply to Loic from comment #9)
> It's not very surprising that the module about scanning SSL traffic is
> involved, in the past, this module has been the source of various issues.

SSL scanning of web traffic is new as of Avast 2015 released Tuesday the 21st.  It was not available before.
It's likely an issue on their side, with DLLs hooking firefox.exe making it crash.
Component: Untriaged → Security
Product: Firefox → Core
Summary: Firefox won't close with Avast 2015 10.0.2206 → Firefox won't close with Avast 2015 10.0.2206 when HTTPS scanning is enabled
Pale Moon 25.0.1 *does not* crash when HTTPS scanning is enabled.
It seems the problem is not only in Avast.
Reproducible: always

Steps To Reproduce:
1. Avast free 2015 10.0.2206 installed (installed all shields , but no tools.)
2. Open Firefox with clean profile and install any addon and restart
3. Open about:addons and, Check for Updates from Gear Icon
4. Quit Browser

Actual Results:
Browser crashes after several time
Status: UNCONFIRMED → NEW
Ever confirmed: true
Users in the Avast forum have reported that the crash does not occur with FF versions prior to 32.
And as I've reported in this post: Pale Moon 25.0.1 *does not* crash when HTTPS scanning is enabled.
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

STR Comment 16

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89073e734ebb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140729073438
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1b0d6b870c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140729115208
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=89073e734ebb&tochange=3c052ee8d76a

Triggered by
3c052ee8d76a	Georg Fritzsche — Bug 1039226 - Trigger explicit OpenH264 updates from OpenH264Provider. r=unfocused
Blocks: 1039226
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox-esr31: --- → unaffected
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
tracking-firefox36: --- → ?
Flags: needinfo?(georg.fritzsche)
Thanks. :)
Tracking because it crashes Firefox.
I guess Avast is unhappy about the OpenH264 actions.
tracking-firefox33: ? → +
tracking-firefox34: ? → +
tracking-firefox35: ? → +
tracking-firefox36: ? → +
Note that crashes occasionally occur without checking for add-ons updates.
Does this happen because FF has checked OpenH264 in the background? Or there might be a problem with other secure sites as well?
Firefox 33.0.1 crashes even with OpenH264 plugin disabled when https scanning is enabled.  The only site I visited during the session was forum.avast.com, and read new responses.  On closing, crash shortly afterwards.
I had updated avast a few days ago, and I use a Toshiba I7 laptop, I had taken firefox off both computers and reinstalled the lattest 33.0.1 version, when I did that yesterday laptop was fine, no crashes, but started crashing again today.  Firefox been crashing since 33.0 and with avast
There was a mention somewhere to disable hardware acceleration, which i did, did not crash for a while, then went back to crashing
This is not a normal crash, this is one of the shutdown timeouts:
{"phase":"profile-before-change","conditions":[{"name":"AddonManager: shutting down providers","state":"(none)","filename":"resource://gre/modules/AddonManager.jsm","lineNumber":744,"stack":["resource://gre/modules/AddonManager.jsm:AMI_startup:744","resource://gre/modules/AddonManager.jsm:AMP_startup:2322","resource://gre/components/addonManager.js:AMC_observe:55","null:null:0"]}]}

Can we please have confirmation if anything after Fx 33 is actually affected?

This is one manifestation of bug 1057312 et al - bug 1057312, comment 22 has details on what was uplifted to 33.0.1, 34 should not be affected at all.
Flags: needinfo?(georg.fritzsche)
(In reply to Yaron from comment #15)
> Pale Moon 25.0.1 *does not* crash when HTTPS scanning is enabled.
> It seems the problem is not only in Avast.

That build probably doesn't have the OpenH264 feature, so this is not surprising.
As this is only happening with Avast, it is likely that they do something that gets our update checks from GMPInstallManager.jsm stuck or makes them throw in an unexpected place.
(In reply to Gopher John from comment #22)
> Firefox 33.0.1 crashes even with OpenH264 plugin disabled when https
> scanning is enabled.  The only site I visited during the session was
> forum.avast.com, and read new responses.  On closing, crash shortly
> afterwards.

Does it still happen when you disable "Automic Updates" in the OpenH264 details?
We do have regular automatic checks for OpenH264 updates too.
Flags: needinfo?(list.jwrgray)
(In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> (In reply to Gopher John from comment #22)


> 
> Does it still happen when you disable "Automic Updates" in the OpenH264
> details?
> We do have regular automatic checks for OpenH264 updates too.

No, it doesn't.  But, I also have to disable Add-on Update Checker 2.7 https://addons.mozilla.org/en-US/firefox/addon/addon-update-checker, which will initiate the check regardless of setting.  I didn't think that this extension would trigger checks for plugins.
Flags: needinfo?(list.jwrgray)
Ah, thanks. Our integration for OpenH264 supports explicit updates like other extensions though, so if an addon triggers that explicitly for all addons regardless of what type they are...

Can someone please check if this happens on 34 or newer too?
https://www.mozilla.org/en-US/firefox/channel/#beta
@:gfritzsche]
FYI Bug 1088471 should be related to this bug.
Bug 1088471 Firefox cannot find addon updates with Avast 2015 10.0.2206 when HTTPS scanning is enabled.

Avast HTTPS scanning interferes Firefox's addon update for some reasons.
See Also: → 1088471
Triggering "find update" for OpenH264, i get this:

Expected certificate attribute 'issuerName' value incorrect, expected: 'CN=DigiCert Secure Server CA,O=DigiCert Inc,C=US', got: 'CN=avast! Web/Mail Shield Root,O=avast! Web/Mail Shield,OU=generated by avast! antivirus for SSL/TLS scanning'.
Expected certificate attribute 'issuerName' value incorrect, expected: 'CN=Thawte SSL CA,O="Thawte, Inc.",C=US', got: 'CN=avast! Web/Mail Shield Root,O=avast! Web/Mail Shield,OU=generated by avast! antivirus for SSL/TLS scanning'.
Certificate checks failed. See previous errors for details. CertUtils.jsm:109
NS_ERROR_ILLEGAL_VALUE: Certificate checks failed. See previous errors for details. CertUtils.jsm:110
So, Avast breaks things here, but our handling there should be better.
I see the above errors, but apparently the update task in the OpenH264Provider doesn't cancel properly.

I assume this is either
* bug 1061611 (the XHR in GMPInstallManager not timing out) or
* the GMPInstallManagers checkForAddons() not rejecting/resolving the returned promise due to unhandled exceptions
Depends on: 1061611
Monica, do you happen to have an idea on how to trigger the failures in comment 32 with a setup that doesn't involve Windows and Avast?
Flags: needinfo?(mmc)
Jorge, can you reach out to Avast on the breakage they cause?
Flags: needinfo?(jorge)
(In reply to Georg Fritzsche [:gfritzsche] from comment #34)
> Monica, do you happen to have an idea on how to trigger the failures in
> comment 32 with a setup that doesn't involve Windows and Avast?

If you MITM yourself with ZAP ( https://www.owasp.org/index.php/OWASP_Zed_Attack_Proxy_Project ) or something (and import/trust the root it generates) and then attempt to update addons, that check should fail.
Flags: needinfo?(mmc)
(In reply to Georg Fritzsche [:gfritzsche] from comment #32)
> Expected certificate attribute 'issuerName' value incorrect, expected:
> 'CN=DigiCert Secure Server CA,O=DigiCert Inc,C=US', got: 'CN=avast! Web/Mail
> Shield Root,O=avast! Web/Mail Shield,OU=generated by avast! antivirus for
> SSL/TLS scanning'.

Sounds a lot like Avast is doing an SSL MITM to scan stuff and we "correctly" block that due to cert pinning (correctly because when we see a cert from a wrong issuer, we should block as it easily could be a real attack).
(In reply to George Ruch from comment #38)
> Re: OpenH264 support.  In the last two (at least) crashes I've had, the
> OpenH264 extension was *not* installed.

It doesnt matter whether it is installed or not, but whether we tried to update/download the list of plugins.

> Watching the process in System
> Explorer, FF sits for quite a while after clearing its history and cache,
> then reports the crash when its process is closed (Delete PID).

This is expected - this is not a real crash, it is our shutdowns wait timing out, then intentionally triggering a crash.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #37)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #32)
> > Expected certificate attribute 'issuerName' value incorrect, expected:
> > 'CN=DigiCert Secure Server CA,O=DigiCert Inc,C=US', got: 'CN=avast! Web/Mail
> > Shield Root,O=avast! Web/Mail Shield,OU=generated by avast! antivirus for
> > SSL/TLS scanning'.
> 
> Sounds a lot like Avast is doing an SSL MITM to scan stuff and we
> "correctly" block that due to cert pinning (correctly because when we see a
> cert from a wrong issuer, we should block as it easily could be a real
> attack).

Indeed - that is their fault. But we shouldn't time out on shutdown due to this.
(In reply to Georg Fritzsche [:gfritzsche] from comment #33)
> I assume this is either
> * bug 1061611 (the XHR in GMPInstallManager not timing out) or
> * the GMPInstallManagers checkForAddons() not rejecting/resolving the
> returned promise due to unhandled exceptions

You mentioning promises also reminds me of bug 1058695 - not sure it's related, but it's an issue with promises keeping us alive on shutdown.

In any case, this is now the #1 crash on 33 release, we should see to get some way to fix it within this week so we can at least have it not happen in the "birthday release" we ship on Nov 11.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #41)
> You mentioning promises also reminds me of bug 1058695 - not sure it's
> related, but it's an issue with promises keeping us alive on shutdown.

We are definitely running into the async shutdown timeout.

> In any case, this is now the #1 crash on 33 release, we should see to get
> some way to fix it within this week so we can at least have it not happen in
> the "birthday release" we ship on Nov 11.

We are checking who can look into this, i can't right now. Does bug 1088471, comment 6 apply here?
At a minimum, we have the option to add a timeout on the XHR at http://hg.mozilla.org/mozilla-central/annotate/c0ddb1b098ec/toolkit/modules/GMPInstallManager.jsm#l347

But also I've asked the networking team for help here because my understanding is that browser shutdown should be shutting down the network stack and cancelling all HTTP requests. AIUI that happens in profile-change-net-teardown and profile-change-teardown, which is called before we crash here in profile-before-change:

http://hg.mozilla.org/mozilla-central/annotate/c0ddb1b098ec/toolkit/xre/nsXREDirProvider.cpp#l907
The bug for the timeout and another possible error source are in comment 33.
Flags: needinfo?(mcmanus)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #43)
> But also I've asked the networking team for help here because my
> understanding is that browser shutdown should be shutting down the network
> stack and cancelling all HTTP requests. AIUI that happens in
> profile-change-net-teardown and profile-change-teardown, which is called
> before we crash here in profile-before-change:
> 

that's right.

and I just put together a https resource that takes 15 seconds to load and tested shutdown with it in progress and shutdown went ok. But that doesn't prove a lot.

The networking thread shutdown sequence basically stuffs a sentinel into the event queue and processes all the events before it and closes all the connections at the end and then stops processing events.

it seems to me that things shutdown with transaction->close() will signal back to the main thread directly, but some things from connection->close() will require another event on the socket thread to get the channel:onstop generated on the main thread. connection->closeTransaction() wouldn't have that problem as it does both - and the patch to use that is trivial.[*]

I'm not too confident though - its possible something in PSM needs that second I/O to clear - and that would correlate more strongly with the nature of this bug (an invalid cert). That patch would be more invasive - and it seems likely that 1061611 would be the sole right answer for a short term release.

[*] coming soon
Flags: needinfo?(mcmanus)
Patrick, have you checked what happens to XHR requests when they are cancelled by shutdown? Are the clients notified? If not, this could explain a number of crashes.

Also, I can add an option to AsyncShutdown `addBlocker` that will let selected shutdown timeout non-fatally. This should be easy to do, although a bit tricky to uplift.
FYI, 33.1 is probably going to build next Friday. I would be more than happy to take a patch for this bug since it is the #1 topcrash in 33.0.X
needinfo regarding comment 46
Flags: needinfo?(mcmanus)
Flags: needinfo?(georg.fritzsche)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #48)
> Really? That doesn't look like a topcrash on AWSDY:
> http://yoric.github.io/are-we-shutting-down-yet-/?version=Firefox+33.0.1&version=Firefox+33.0#

Erm, how do you come to that conclusion? I see "AddonManager: shutting down providers" as by far the #1 there and everything in here clearly points to crashes that actually show exactly that AsycShutdown name in the details. Now we know there can be different underlying issues ending up with that same message, but that one clearly is by far the #1 topcrash in this category.
the more I think about this - the less the networking connection timeout code is interesting. I mean, the above comments seem to suggest we have identified that the transaction is being mitm'd, so its not going to be just hanging out as an active transaction during shutdown.

the gmp code sets up the xhr and a notification callback called a badcerthandler here:
http://hg.mozilla.org/mozilla-central/annotate/c360f3d1c00d/toolkit/modules/GMPInstallManager.jsm#l357

I believe that's this code:
https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/CertUtils.jsm#189

that code claims to implement nsIBadCertListener - but as far as I can tell does not do so. heck, as far as I can tell nsIBadCertListener no longer exists (nsIBadCertListener2 does - this would be a psm question for :keeler to get an authoritative answer)

is the real core problem here that the xhr doesn't complete because the xhr isn't canceled because the cert failure isn't handled right?
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #46)
> Patrick, have you checked what happens to XHR requests when they are
> cancelled by shutdown? Are the clients notified? If not, this could explain
> a number of crashes.
> 

I will go test that case - my case wasn't an xhr. The xhr interaction with necko is actually maintained in the dom code, generally by :bz - so we might need to get him involved.

but as comment 51 says, it seems weird that shutdown is actually the root of this bug.. the avast plugin (at least according to how I read the comment trail) isn't making any networking hang.. its just generating a detected mitm. but maybe its having that effect too.
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #45)
>onnection->closeTransaction() wouldn't have
> that problem as it does both - and the patch to use that is trivial.[*]
> 
> 
> 
> [*] coming soon

this is a bit of a shot in the dark - but the patch is nice and slim. Can someone who can reproduce the bug try the builds here just to see if it helps?

   https://tbpl.mozilla.org/?tree=Try&rev=6d5dd2f32231
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #50)
> Erm, how do you come to that conclusion? I see "AddonManager: shutting down
> providers" as by far the #1 there and everything in here clearly points to
> crashes that actually show exactly that AsycShutdown name in the details.
> Now we know there can be different underlying issues ending up with that
> same message, but that one clearly is by far the #1 topcrash in this
> category.

We might be talking of different things. Indeed, Add-on manager related stuff is #1 AsyncShutdown crash for 33.0.*, with 92% of the AsyncShutdown crash reports. Looking at crash signatures, however, some have Avast, some don't (I'm looking for asw*.dll in the modules, if anybody can tell me  better way to look for Avast, I'm open to suggestions). As you can see on http://yoric.github.io/are-we-shutting-down-yet-/?signature=~AddonManager%3A+shutting+down+providers#, these crashes have stopped at Firefox 33, presumably solved by bug 1079312, which has been uplifted to 33 a few days ago.

Now, OpenH264 crashes affect every version of Firefox including the latest Nightly: http://yoric.github.io/are-we-shutting-down-yet-/?signature=~OpenH264Provider#, so they are clearly not fixed by the same patch, nor code by the same high-level codepath. Since they represent only 6% of the AsyncShutdown crash reports, they don't seem #1 to me. We clearly need to fix this, but I am a bit surprised by the "#1 topcrash" classification.
(In reply to Patrick McManus [:mcmanus] from comment #51)
> the more I think about this - the less the networking connection timeout
> code is interesting. 

I've put together a test that does an xhr to a resource with a bad cert (the cert is signed for a cname of the resource I use in the test). This seems similar to what avast is doing (the details of why the cert validation fails are different.. I don't think that matters here).

From the set of events { err, load, progress, abort, loadend} I see err and loadend fire on the xhr in that scenario.

I'm not an expert on the xhr documentation, but from a quick read that seems like the right thing.

hg.mozilla.org/mozilla-central/annotate/c360f3d1c00d/toolkit/modules/GMPInstallManager.jsm#l357 seems to have event handlers for only load and err.

I can't really follow the logic of what happens in onErrorXML - is it possible that it isn't fatal to the transaction if a load event never occurs?
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #54)
> Now, OpenH264 crashes affect every version of Firefox including the latest
> Nightly:
> http://yoric.github.io/are-we-shutting-down-yet-/
> ?signature=~OpenH264Provider#, so they are clearly not fixed by the same
> patch, nor code by the same high-level codepath. Since they represent only
> 6% of the AsyncShutdown crash reports, they don't seem #1 to me. We clearly
> need to fix this, but I am a bit surprised by the "#1 topcrash"
> classification.

I'm not sure if that classification catches all those issues as I'm not sure how you generate the classification itself, but actually, for the small sample that is 33.0.1 and 33.0.2, it clearly is the #1 issue: http://yoric.github.io/are-we-shutting-down-yet-/?version=Firefox+33.0.1&version=Firefox+33.0.2#
And as bug 1074135 landed in 33.0.1 (build #2), it's clear that we can have a significant change in what is shown in those reports. I guess that change only made it stick out that OpenH264Provider actually is the main thing behind the AsyncShutdown crashes that have been plaguing 33 release.
ni re comment 55 and the onerrorxml path
Regarding the number of crash reports: many Avast users have disabled "HTTPS Scanning" in the last few days.
By the way,Could mozilla describe the crash problem and a workaround to release note?
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Using_XMLHttpRequest

"One can also detect all three load-ending conditions (abort, load, or error) using the loadend event:"

if we take that at its word, its a problem that you don't have a handler for abort (or at least a loadend catch all).. and maybe that error is assuming load will also be called? Either of those could explain what we're seeing. :bz would know when error was called vs abort.
So we can:
1. add an `abort` or `loadend` handler in that xhr;
2. wrap that xhr in a non-fatal timeout;
3. extend AsyncShutdown's `addBlocker` to support non-fatal timeout.

Given the short amount of time we have for testing, I believe that we should do both 1. and 2. Option 3. would be useful, but probably hard to uplift.

Also, KaiRo convinced me over IRC that Avast is a good candidate for most of the AsyncShutdown crashes that we see on AWSDY.
Assuming that the error is indeed in or around that XHR, I'll write a patch.
Assignee: nobody → dteller
If the error was diagnosed correctly, this should fix it.
Attachment #8513441 - Flags: review?(georg.fritzsche)
Applied irc feedback.
This passes tests, but I do not have the possibility of checking whether it solves the actual issue today. If someone could do it, this would be extremely useful.

Try and binaries: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6515b7dc0af7

Note: I have attempted to install Zap, and this doesn't cause the same issue as Avast.
Attachment #8513441 - Attachment is obsolete: true
Attachment #8513441 - Flags: review?(georg.fritzsche)
Attachment #8513552 - Flags: review?(georg.fritzsche)
(In reply to Alice0775 White from comment #60)
> By the way,Could mozilla describe the crash problem and a workaround to
> release note?

Sure. Thanks for the idea. Done with "Windows: With Avast HTTPS scanning enabled, Firefox will not close correctly (1087674)" as wording.
relnote-firefox: --- → 33+
so I now have a vm where I can repro the problem and I tried the builds from comment 65 as a fix. The fix didn't work. The builds in comment 66 weren't ready for testing yet, but if I understand it correctly they just change the logging, correct?
No, not just logging, they change the timeout machinery, too.
If this is not sufficient, then the xhr is not the [only?] culprit.
Oops, wrong patch.
Attachment #8513552 - Attachment is obsolete: true
Attachment #8513552 - Flags: review?(georg.fritzsche)
Attachment #8513605 - Flags: review?(georg.fritzsche)
yoric - are the builds in 66 based on v2 or v3?
the builds from comments 66 and 53 don't help either.

now that I have a repro I'll look into other causes again.
Build from 66 is v2 (for logging) + v3 (for the expected fix).
I'll look into other causes, too, in a few hours.
fwiw onloadxml does fire thanks to the new logging in build from 66
it is possible we need to catch the exception from checkCert in onLoadXML and call onFailXML from it? (or at least do this._deferred.reject()) It is definitely throwing.

I don't have a windows build setup and the vm has horrible IO properties.. so its hard for me to test it without waiting for try (which I'll do). I unzipped omni.ja and edited the js directly, rezipped it .. but somehow my changes don't seem to be taking effect. pointers welcome.
(In reply to Patrick McManus [:mcmanus] from comment #74)
> fwiw onloadxml does fire thanks to the new logging in build from 66

it took me a minute to figure out that it makes sense.. avast annotates the trust anchor list - that's why normal https browsing doesn't complain about being mitm'd.. so we would expect the xhr to return the successful case (onloadxml). In the load handler is where we double check the cert against some kind of pin criteria and find the mismatch - which throws NS_ERROR_ILLEGAL_VALUE
By the way, the "pinning" CertUtils does was never robust. Now that we have actual pinning, we should remove the CertUtils code and rely on pinning (see also bug 1002586).
(In reply to Patrick McManus [:mcmanus] from comment #75)
> I unzipped omni.ja and edited the js directly, rezipped it .. but somehow my
> changes don't seem to be taking effect. pointers welcome.

If there's an equivalent pre-compiled file, you should delete it for changes to take effect.
Yes, that's also my conclusion: we have an uncaught exception. I'll provide a patch asap.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #77)
> By the way, the "pinning" CertUtils does was never robust. Now that we have
> actual pinning, we should remove the CertUtils code and rely on pinning (see
> also bug 1002586).

This was apparently special due to using aus4, please comment bug 1052365 if that situation changed.
It definitely is not something we should look into for uplift.
Flags: needinfo?(georg.fritzsche)
Also, without the fix, the test I have just added timeouts, which matches the symptoms we encounter.
I can confirm :yoric's builds from comment 81 fix the avast problem on my win 7 vm.

I can also confirm that another patchset I wrote (https://tbpl.mozilla.org/?tree=Try&rev=1da4c27c3ff3) that did basically the same thing also fixes the problem.

looks resolved.
Comment on attachment 8513783 [details] [diff] [review]
Making GMPInstallManager more resilient wrt SSL certification error

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

r+ with the below fixed.

::: toolkit/modules/GMPInstallManager.jsm
@@ +536,5 @@
> +
> +      gCertUtils.checkCert(this._request.channel, allowNonBuiltIn, certs);
> +
> +      this.parseResponseXML();
> +    } catch (ex) {

We should log.error() here or at least Cu.reportError().

@@ +537,5 @@
> +      gCertUtils.checkCert(this._request.channel, allowNonBuiltIn, certs);
> +
> +      this.parseResponseXML();
> +    } catch (ex) {
> +      this._deferred.reject(ex);

Please fix this to reject with an object conforming to the format documented for GMPInstallManager.checkForAddons().
Attachment #8513783 - Flags: review?(georg.fritzsche) → review+
Comment on attachment 8513605 [details] [diff] [review]
Making GMPInstallManager more resilient wrt abort() / timeout, v3

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

r+ with comments addressed.

::: toolkit/modules/GMPInstallManager.jsm
@@ +63,5 @@
> + * antivirus behavior). This timeout is a defensive measure to ensure
> + * that we fail cleanly in such case.
> + *
> + * This value is defined as a `let`, not a `const`, to allow
> + * overriding from the test suite.

I don't see it actually being overridden anywhere?
Lets make it `const`, we can still change that when actually needed.

@@ +384,5 @@
>  
> +    this._request.timeout = CHECK_FOR_ADDONS_TIMEOUT_DELAY_MS;
> +    this._request.addEventListener("error", this.onFailXML.bind(this, "onErrorXML") ,false);
> +    this._request.addEventListener("abort", this.onFailXML.bind(this, "onAbortXML") ,false);
> +    this._request.addEventListener("timeout", this.onFailXML.bind(this, "onTimeoutXML") ,false);

* Preference for arrow-functions here for readability.
* s/ ,false/, false/
* Should we move any or all of the listener registrations to before xhr.open()?

@@ +580,5 @@
>    /**
> +   * The XMLHttpRequest took too long and was aborted by
> +   * a defensive timeout.
> +   */
> +  onTimeoutXML: function() {

This seems unused.
Attachment #8513605 - Flags: review?(georg.fritzsche) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #35)
> Jorge, can you reach out to Avast on the breakage they cause?

I'll get in touch with our known contacts.
Flags: needinfo?(jorge)
(In reply to Jorge Villalobos [:jorgev] from comment #86)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #35)
> > Jorge, can you reach out to Avast on the breakage they cause?
> 
> I'll get in touch with our known contacts.

Could you suggest that the best way to add a feature to Firefox is to contribute a xpcom component, rather than attempting to hack through our encryption layer?
Comment on attachment 8514178 [details] [diff] [review]
Making GMPInstallManager more resilient to abort(), timeout or certificate errors

Approval Request Comment
[Feature/regressing bug #]: Bug 1039226, but triggered by a new version of Avast that attempts to hack through our SSL communications.
[User impact if declined]: topcrasher #1 under Windows, during shutdown.
[Describe test coverage new/current, TBPL]: TBPL is https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4d80a4d17662 and https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=418c0acd10ff, manual testing confirms that the patch fixes the issue. The patch is low-risk, but depending on the schedule for 33.1, I would prefer if we could let the patch cool down a few days on Nightly before uplifting.
[Risks and why]: None that I can think of.
[String/UUID change made/needed]: None.
Attachment #8514178 - Flags: approval-mozilla-release?
Attachment #8514178 - Flags: approval-mozilla-beta?
Attachment #8514178 - Flags: approval-mozilla-aurora?
Comment on attachment 8514178 [details] [diff] [review]
Making GMPInstallManager more resilient to abort(), timeout or certificate errors

Despite David's suggestion, I am going to take it for every version right now:
* We need a new quick 33.1 build to have QE verify it
* 33.1 is not going to be push to the normal channel. So, the impact should be limited to our testers. We can always backout it if anything goes wrong
* Pushing it to aurora & beta won't help us much with avast (we didn't get this bug report before)... but it might help us to find side effects
Attachment #8514178 - Flags: approval-mozilla-release?
Attachment #8514178 - Flags: approval-mozilla-release+
Attachment #8514178 - Flags: approval-mozilla-beta?
Attachment #8514178 - Flags: approval-mozilla-beta+
Attachment #8514178 - Flags: approval-mozilla-aurora?
Attachment #8514178 - Flags: approval-mozilla-aurora+
I agree with the decision to take this on 33.1  but just wanted to clarify one point

(In reply to Sylvestre Ledru [:sylvestre] from comment #91)
>
> * Pushing it to aurora & beta won't help us much with avast (we didn't get
> this bug report before)... but it might help us to find side effects

the avast trigger wasn't released by avast until mid october... so that doesn't really reflect on whether the pre-release audience would have shown this.
Ok, thanks for the feedback Patrick!
https://hg.mozilla.org/mozilla-central/rev/1b80d1642142
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox36: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
On behalf of many Firefox and Avast users: Thank you. We appreciate your work. :)

Does 33.1 RC build 2 include the patch?
(In reply to Yaron from comment #96)
> Does 33.1 RC build 2 include the patch?
Yes, don't hesitate to confirm that the issue is fixed for you!
I can't seem to be able to reproduce the crash with the latest Avast (all shields on) and Firefox 33.0.1, on Win 7 x64. Alice/Yaron, could you perhaps confirm that this fixes the problem on:
- Firefox 33.1 build 2 - https://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/33.1-candidates/build2/
and
- Firefox 34 beta 5 - ftp://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/34.0b5-candidates/build2/
I cannot reproduce the problem anymore on the following builds.
Seems Avast! or AMO has changed something.

https://hg.mozilla.org/releases/mozilla-esr24/rev/63ce8133f1cc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 ID:20140923194127

https://hg.mozilla.org/releases/mozilla-esr31/rev/d14010cafcab
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20141011074935

https://hg.mozilla.org/releases/mozilla-release/rev/7dc4a9d1b3e6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20141027150301

https://hg.mozilla.org/releases/mozilla-beta/rev/6cb4888905c9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20141027152126

https://hg.mozilla.org/releases/mozilla-aurora/rev/38ef8541c3fc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141027004000

https://hg.mozilla.org/mozilla-central/rev/80e18ff7c7b2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141030030218
(In reply to Sylvestre Ledru [:sylvestre] from comment #97)
> Yes, don't hesitate to confirm that the issue is fixed for you!
Fixed in 33.1.
Thanks again to all contributors.
Thanks for the fix of Firefox in upcoming releases.  Good work on a quick fix.

Avast did their part for Firefox 33.0.x. by removing the trigger event.
Quote from: lukor on Today at 03:54:39 AM

    - fixed in Avast via VPS - URLs used for extension update check are not scanned - should not crash with Firefox 33.0 anymore

    Lukas.
Unquote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: