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)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: mvocom, Assigned: Yoric)
References
Details
(Keywords: crash)
Crash Data
Attachments
(2 files, 4 obsolete files)
14.43 KB,
patch
|
Yoric
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
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.
Comment 2•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
All these are from one Firefox profile. First crashes https://crash-stats.mozilla.com/report/index/bp-28a94bf3-2ed4-44c2-8d6a-565572141021 https://crash-stats.mozilla.com/report/index/bp-d0f3e3e5-6637-4d86-8282-d95162141021 https://crash-stats.mozilla.com/report/index/bp-df9deb50-2298-4f4a-9ac9-370ff2141022 Last three. https://crash-stats.mozilla.com/report/index/bp-759d673b-c506-4c68-8f60-fa2c42141023 https://crash-stats.mozilla.com/report/index/bp-072614ae-cbd0-4188-979d-6f9082141023 https://crash-stats.mozilla.com/report/index/bp-e1ccebed-f6c7-49a9-abcb-194022141023 https://crash-stats.mozilla.com/report/index/bp-6f3a9b79-aa42-485d-9f3d-77d1d2141022 https://crash-stats.mozilla.com/report/index/bp-bea7dd38-b4a5-4e0e-ba62-fb8e42141022
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?
Comment 6•10 years ago
|
||
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.
Reporter | ||
Comment 10•10 years ago
|
||
Thank you. What's next? :)
Comment 11•10 years ago
|
||
Contact the support of Avast.
Reporter | ||
Comment 12•10 years ago
|
||
We've done that. Thanks.
Comment 13•10 years ago
|
||
(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.
Comment 14•10 years ago
|
||
It's likely an issue on their side, with DLLs hooking firefox.exe making it crash.
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
Reporter | ||
Comment 15•10 years ago
|
||
Pale Moon 25.0.1 *does not* crash when HTTPS scanning is enabled. It seems the problem is not only in Avast.
Comment 16•10 years ago
|
||
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
Reporter | ||
Comment 17•10 years ago
|
||
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.
Comment 18•10 years ago
|
||
[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)
Reporter | ||
Comment 19•10 years ago
|
||
Thanks. :)
Comment 20•10 years ago
|
||
Tracking because it crashes Firefox. I guess Avast is unhappy about the OpenH264 actions.
Reporter | ||
Comment 21•10 years ago
|
||
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?
Comment 22•10 years ago
|
||
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.
Comment 23•10 years ago
|
||
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
Comment 24•10 years ago
|
||
There was a mention somewhere to disable hardware acceleration, which i did, did not crash for a while, then went back to crashing
Comment 25•10 years ago
|
||
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)
Comment 26•10 years ago
|
||
(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.
Comment 27•10 years ago
|
||
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.
Comment 28•10 years ago
|
||
(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)
Comment 29•10 years ago
|
||
(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)
Comment 30•10 years ago
|
||
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
Comment 31•10 years ago
|
||
@: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.
Comment 32•10 years ago
|
||
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
Comment 33•10 years ago
|
||
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
Comment 34•10 years ago
|
||
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)
Comment 35•10 years ago
|
||
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.
Updated•10 years ago
|
Flags: needinfo?(mmc)
Comment 37•10 years ago
|
||
(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).
Comment hidden (spam) |
Comment 39•10 years ago
|
||
(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.
Comment 40•10 years ago
|
||
(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.
Comment 41•10 years ago
|
||
(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.
Comment 42•10 years ago
|
||
(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?
Comment 43•10 years ago
|
||
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
Comment 44•10 years ago
|
||
The bug for the timeout and another possible error source are in comment 33.
Updated•10 years ago
|
Flags: needinfo?(mcmanus)
Comment 45•10 years ago
|
||
(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)
Assignee | ||
Comment 46•10 years ago
|
||
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.
Comment 47•10 years ago
|
||
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
Assignee | ||
Comment 48•10 years ago
|
||
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#
Assignee | ||
Comment 49•10 years ago
|
||
needinfo regarding comment 46
Flags: needinfo?(mcmanus)
Flags: needinfo?(georg.fritzsche)
Comment 50•10 years ago
|
||
(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.
Comment 51•10 years ago
|
||
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?
Comment 52•10 years ago
|
||
(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)
Comment 53•10 years ago
|
||
(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
Assignee | ||
Comment 54•10 years ago
|
||
(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.
Comment 55•10 years ago
|
||
(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?
Comment 56•10 years ago
|
||
(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#
Comment 57•10 years ago
|
||
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.
Comment 58•10 years ago
|
||
ni re comment 55 and the onerrorxml path
Reporter | ||
Comment 59•10 years ago
|
||
Regarding the number of crash reports: many Avast users have disabled "HTTPS Scanning" in the last few days.
Comment 60•10 years ago
|
||
By the way,Could mozilla describe the crash problem and a workaround to release note?
Comment 61•10 years ago
|
||
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.
Assignee | ||
Comment 62•10 years ago
|
||
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.
Assignee | ||
Comment 63•10 years ago
|
||
Assuming that the error is indeed in or around that XHR, I'll write a patch.
Assignee: nobody → dteller
Assignee | ||
Comment 64•10 years ago
|
||
If the error was diagnosed correctly, this should fix it.
Attachment #8513441 -
Flags: review?(georg.fritzsche)
Assignee | ||
Comment 65•10 years ago
|
||
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f35ae3afb266
Assignee | ||
Comment 66•10 years ago
|
||
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)
Comment 67•10 years ago
|
||
(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+
Comment 68•10 years ago
|
||
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?
Assignee | ||
Comment 69•10 years ago
|
||
No, not just logging, they change the timeout machinery, too. If this is not sufficient, then the xhr is not the [only?] culprit.
Assignee | ||
Comment 70•10 years ago
|
||
Oops, wrong patch.
Attachment #8513552 -
Attachment is obsolete: true
Attachment #8513552 -
Flags: review?(georg.fritzsche)
Attachment #8513605 -
Flags: review?(georg.fritzsche)
Comment 71•10 years ago
|
||
yoric - are the builds in 66 based on v2 or v3?
Comment 72•10 years ago
|
||
the builds from comments 66 and 53 don't help either. now that I have a repro I'll look into other causes again.
Assignee | ||
Comment 73•10 years ago
|
||
Build from 66 is v2 (for logging) + v3 (for the expected fix). I'll look into other causes, too, in a few hours.
Comment 74•10 years ago
|
||
fwiw onloadxml does fire thanks to the new logging in build from 66
Comment 75•10 years ago
|
||
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.
Comment 76•10 years ago
|
||
(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).
Reporter | ||
Comment 78•10 years ago
|
||
(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.
Assignee | ||
Comment 79•10 years ago
|
||
Yes, that's also my conclusion: we have an uncaught exception. I'll provide a patch asap.
Comment 80•10 years ago
|
||
(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)
Assignee | ||
Comment 81•10 years ago
|
||
Try and binaries: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d0ea2c158c85
Attachment #8513783 -
Flags: review?(georg.fritzsche)
Assignee | ||
Comment 82•10 years ago
|
||
Also, without the fix, the test I have just added timeouts, which matches the symptoms we encounter.
Comment 83•10 years ago
|
||
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 84•10 years ago
|
||
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 85•10 years ago
|
||
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+
Comment 86•10 years ago
|
||
(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)
Assignee | ||
Comment 87•10 years ago
|
||
(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?
Assignee | ||
Comment 88•10 years ago
|
||
Folded patches, applied feedback. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4d80a4d17662
Attachment #8513605 -
Attachment is obsolete: true
Attachment #8513783 -
Attachment is obsolete: true
Attachment #8514178 -
Flags: review+
Assignee | ||
Comment 89•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=418c0acd10ff
Attachment #8514189 -
Flags: review+
Assignee | ||
Comment 90•10 years ago
|
||
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 91•10 years ago
|
||
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+
Comment 92•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1b80d1642142 https://hg.mozilla.org/releases/mozilla-aurora/rev/9b7a09c5d8bc https://hg.mozilla.org/releases/mozilla-beta/rev/60b312ec8c87 https://hg.mozilla.org/releases/mozilla-release/rev/0de6e2b5507a
Whiteboard: [fixed-in-fx-team]
Comment 93•10 years ago
|
||
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.
Comment 94•10 years ago
|
||
Ok, thanks for the feedback Patrick!
Comment 95•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1b80d1642142
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Reporter | ||
Comment 96•10 years ago
|
||
On behalf of many Firefox and Avast users: Thank you. We appreciate your work. :) Does 33.1 RC build 2 include the patch?
Comment 97•10 years ago
|
||
(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!
Comment 98•10 years ago
|
||
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/
Comment 99•10 years ago
|
||
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
Reporter | ||
Comment 100•10 years ago
|
||
(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.
Comment 101•10 years ago
|
||
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
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
You need to log in
before you can comment on or make changes to this bug.
Description
•