1268025 - startup crashes by HitmanPro Alert after the firefox 46 update

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine: JIT, defect)

Description

[[:philipp]]

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-8506637d-e74e-4644-9a84-cc8e12160427.
=============================================================
Frame 	Module 	Signature 	Source
0 		@0x9cf0300 	
1 		@0x1c560967 	
2 	xul.dll 	EnterBaseline 	js/src/jit/BaselineJIT.cpp
3 	xul.dll 	js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) 	js/src/jit/BaselineJIT.cpp
4 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp
5 	xul.dll 	js::InternalGCMethods<jsid>::isMarkable(jsid) 	js/src/gc/Barrier.h
6 		@0x1

we are seeing various reports of users HitmanPro Alert which firefox started to crash repeatedly or at startup after they have updated to version 46.0. 
those crash reports generally show "EXCEPTION_ACCESS_VIOLATION_EXEC" as crash reason and "hmpalert.dll" loaded in the list of dlls. correlation data is not yet in for 46.0, so maybe this is at the root of an increase of various other crash signatures as well.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Given the stack, I'll change the component to hopefully get the right eyes on it.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like this may be a topcrash on 46 (very early data though)  These signatures also look like high volume crashes on 45.0.2. I"m not sure if this is new behavior.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Naveed can someone on your team look this crash? It is a topcrash on 46 release and seems from user comments to be a new issue in 46. 
n-i to jandem who was looking at possibly similar crashes recently.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

Looks like a new version of HitmanPro was released to address these issues: http://www.wilderssecurity.com/threads/hitmanpro-alert-support-and-discussion-thread.324841/page-383#post-2583777

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #4)
> Looks like a new version of HitmanPro was released to address these issues:
> http://www.wilderssecurity.com/threads/hitmanpro-alert-support-and-
> discussion-thread.324841/page-383#post-2583777

It looks like HitmanPro didn't play well with the W^X protection we added for JIT code. Some posts in that thread complain about slowness as well, but that has also been addressed, looks like.

Since our code is correct AFAIK, the only options we have are:

(1) Do a point release to disable W^X in 46. That'd be unfortunate.
(2) Hope most users will get the Hitman Pro update soon.

Note that the EnterBaseline signature is pretty generic and won't go away completely either way. It'd be interesting to see what % of EnterBaseline crashes have hmpalert.dll

Comment 6

[[:philipp]]

these are the correlations of hmpalert.dll involvement in yesterday's 46.0 crashes:

  js::jit::EnterBaselineMethod|EXCEPTION_ACCESS_VIOLATION_EXEC (456 crashes)
     94% (427/456) vs.   5% (817/15315) hmpalert.dll

  EnterBaseline|EXCEPTION_ACCESS_VIOLATION_EXEC (190 crashes)
    100% (190/190) vs.   5% (817/15315) hmpalert.dll

  js::RegExpShared::execute|EXCEPTION_ACCESS_VIOLATION_EXEC (137 crashes)
    100% (137/137) vs.   5% (817/15315) hmpalert.dll

  js::jit::Linker::newCode<T>|EXCEPTION_ACCESS_VIOLATION_WRITE (19 crashes)
     58% (11/19) vs.   5% (817/15315) hmpalert.dll

so over all the issue was at the root of 5% of all crashes on release. 

none of those crashes were recorded with involvement of hmpalert.dll 3.1.9.368 or higher in the correlation data yet (that is still very low volume). perhaps we could try to take a shot of blocklisting earlier versions of that dll if newer correlation data confirms that the issue is addressed with hmpalert.dll 3.1.9.368.

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

According to a post on the Dutch website tweakers.net by the HitmanPro dev who solved the issue, users should be getting updated to the new version automatically [1]. The fix apparently backported functionality from HitmanPro 3.5 to 3.1.

[1] http://tweakers.net/downloads/37095/mozilla-firefox-460.html?showReaction=8488919#r_8488919

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

Looks like HitMan Pro still affects benchmarks even after the fix on their end. Comparing the 32-bit builds on Windows 10, SunSpider is particularly affected. The difference on Octane may not be as bad as it looks: I got occasional lower scores without HitmanPro as well (not shown below), so 46.0 may be close to bimodal or trimodal behavior on my PC (the lower modes being around 32000 and 33500). With HitmanPro, it may simply be getting the lowest mode all the time, without actually being all *that* much slower. But that's conjecture on my part.

With HitmanPro:
  Firefox 45.0.2:
    SunSpider:
      162.8ms +/- 2.1%
      165.6ms +/- 3.6%
      164.7ms +/- 2.9%
      160.8ms +/- 1.8%
      162.8ms +/- 3.2%
    Octane:
      34883
      34673
      34788
      35126
      35008
  Firefox 46.0:
    SunSpider:
      250.8ms +/- 1.7%
      253.9ms +/- 3.7%
      249.2ms +/- 1.9%
      247.0ms +/- 1.4%
      247.9ms +/- 2.2%
    Octane:
      31322
      31122
      31315
      31274
      31484

Without HitmanPro:
  Firefox 45.0.2:
    SunSpider:
      162.2ms +/- 3.4%
      158.6ms +/- 2.9%
      165.3ms +/- 2.7%
      157.6ms +/- 1.5%
      157.1ms +/- 0.5%
    Octane:
      35345
      35208
      35134
      35059
      35182
  Firefox 46.0:
    SunSpider:
      165.1ms +/- 3.1%
      167.3ms +/- 5.0%
      164.7ms +/- 2.4%
      166.0ms +/- 1.3%
      163.4ms +/- 2.2%
    Octane:
      34483
      34547
      34697
      34758
      34815

Comment 9

[Naveed Ihsanullah [:naveed]]

Since this crash is corrected by an updated version of their plugin are we good for the bug? The perf issue feels like a potentially different issue with a different priority.

Comment 10

[Jan de Mooij [:jandem]]

Philipp, can you confirm this is mostly fixed now, after HitmanPro released an update for this problem?

Comment 11

[[:philipp]]

yes, in the first days of the 46 release we had 700-800 js crashes with involvement of hmpalert.dll - that is currently down to 60 per day & still with older versions of hitman pro only. 
so i guess we can call this bug fixed by a third-party update...

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Can we blocklist the older version?

Comment 13

[[:philipp]]

Attachment: bug1268025_blocklist.patch

potential blocklisting patch for old crashy versions of hmpalert.dll

Comment 14

[[:philipp]]

Comment on attachment 8750344 [details] [diff] [review]
bug1268025_blocklist.patch

the patch had an obvious mistake & it should have been "MAKE_VERSION(3, 1, 9, 367)". 
however even if that is corrected, in a local testbuild it appears our normal blocklisting approach doesn't work, as the .dll hooks into firefox before our blocklisting code could take care of it...