1151506 - startup crash in xptiInterfaceEntry::GetConstantCount with randomly hex-code named DLL

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Robert Kaiser]

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-1c6bf243-1768-41be-a6ff-792ee2150405.
=============================================================

This startup crash is the largest startup crash in 39 Dev Edition right now, with >5% of all crashes.

Those crashes all have randomly-named DLLs on the stack like 09c3ffd6f1a34fde86e1.dll or cc70bb3b2ef142de8fdb.dll in addition to ones with the longer name that starts in the same hey code, like 09c3ffd6f1a34fde86e1d448e8559c21.dll or cc70bb3b2ef142de8fdb656b2089cabf.dll

This sounds a lot like malware. The signature so far is only hitting 39 and 40, it started on nightly with the 3/28 build, apparently.

Comment 1

[Robert Kaiser]

David, any idea if we can do something there?

Comment 2

[(Away)]

|count| is null at xptiInterfaceInfo::GetConstantCount. 

Our blocklist won't be useful here. At best we could treat the symptoms with a null check. Who knows if the app will just crash somewhere else.

Comment 3

[(Away)]

Attachment: bandaid

Nathan are you the right reviewer for this file?

I admit this is a dodgy fix. I don't have any better ideas though. Suggestions welcome.

Comment 4

[Nathan Froyd [:froydnj]]

Yuck.  One wonders if the malware authors even test their malware...

It looks like this is:

http://www.herdprotect.com/09c3ffd6f1a34fde86e1.dll-01293ce891e46edd81bb2baa0640f7e3a863f528.aspx

"Part of the Yontoo adware component, a web browser plugin that injects unwanted ads in the browser. The module 09c3ffd6f1a34fde86e1.dll by Girafarri has been detected as adware by 33 anti-malware scanners. It will plug into the web browser and display context-based advertisements by overwriting existing ads or by inserting new ones on various web pages."

Instead of this check, could we extend our blocklist check for hex dlls here:

http://mxr.mozilla.org/mozilla-central/source/mozglue/build/WindowsDllBlocklist.cpp#571

to handle DLLs that are only composed of hex characters?

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8588658 [details] [diff] [review]
bandaid

Canceling review while we discuss a better solution.

Comment 6

[(Away)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> Instead of this check, could we extend our blocklist check for hex dlls here:
> 
> http://mxr.mozilla.org/mozilla-central/source/mozglue/build/
> WindowsDllBlocklist.cpp#571
> 
> to handle DLLs that are only composed of hex characters?

Last time we did such a thing, bsmedberg was pretty concerned about false positives. Flagging for an opinion.

Comment 7

[Nathan Froyd [:froydnj]]

Non-malware folks are actually using DLLs with 80-bit+ hex names?  Ugh.

Comment 8

[Benjamin Smedberg]

I was in particular worried about <hex{8}>.* names accidentally conflicting. I'm relatively fine with blocking <hex{16,20}>.*

Comment 9

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

Tracking for 39+ since this is a topcrash.

Comment 10

[(Away)]

I'm not happy about the direction we're headed in with these heuristics. We can't hope to be an antimalware engine. I'll do this patch as recommended, but I hope we'll be able to deal with these with self-support in the future.

Comment 11

[(Away)]

Attachment: block long hex names

Comment 12

[Benjamin Smedberg]

Comment on attachment 8589317 [details] [diff] [review]
block long hex names

I'm guessing an automated test for this would be magically difficult?

Comment 13

[(Away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5250df3b304

FWIW I tested a bunch of edge cases locally.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5250df3b304

Comment 15

[(Away)]

Comment on attachment 8589317 [details] [diff] [review]
block long hex names

Approval Request Comment
[Feature/regressing bug #]: external application
[User impact if declined]: startup crashes
[Describe test coverage new/current, TreeHerder]: green on m-i, local testing
[Risks and why]: Main risk is if a legitimate binary matches this hex pattern. We looked into this for a previous bug and there weren't any.
[String/UUID change made/needed]: none

Comment 16

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

Comment on attachment 8589317 [details] [diff] [review]
block long hex names

Approved for uplift to aurora. Sounds like this is not ideal but is the best option for now to prevent a high volume startup crash caused by malware.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1edabd405ab8

Comment 18

[[:philipp]]

this signature is returning in large amounts in 39.0b7

Comment 19

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

This is the #2 topcrash for 39.0b7. Re-opening.

Comment 22

[Robert Kaiser]

This is still a 96% startup crash in 39.0b7, and it's #1 with 50% of all crashes in that version. This is *really* bad right now.

Comment 23

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

David is there any chance you can look into this?

Comment 24

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

Nathan since you worked on this bug earlier is this something you could help with now? Thanks.

Comment 25

[Nathan Froyd [:froydnj]]

Assuming I'm looking at the right place, it looks like this is coming from different DLLs than it was before, so these DLLs aren't getting blocked.  Maybe they wised up to our blocking and they renamed their DLLs?

I can't get the correlations to load, so I can't get a sense of how many crashes are due to which DLLs.

This is looking like the next step in the arms race. :(

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Presumably this is the result of the nsIInterfaceInfo.idl change in https://hg.mozilla.org/mozilla-central/rev/1652874e2d97 ?  Should we consider changing that in some way, e.g., by moving the new method to the end?

Comment 27

[Benjamin Smedberg]

Yeah, formerly it was hex, now it's random-ASCII: Xmnjnyfx.dll

What's weird is I can't figure out why they'd be working around the block when it's just going to crash.

Changing the ordering of the IDL from bug 997325 might resolve this, but also might cause problems with other addons. It would be pretty risk.

If this is a topcrash, why is it that we can't get an actual sample of this malware DLL? That would make it a lot easier to debug/figure out mitigation strategies.

Comment 28

[(Away)]

In many reports this file appears in a directory called "Pay-By-Ads\Yahoo! Search" but at other times it shows up in directories like "btclient" or "dlclient" so it's probably a common library.

We may want to try talking to the Pay-By-Ads developers to see if they develop this library or could tell us where it came from. (I don't have high hopes though)

Comment 29

[(Away)]

My original proposal from attachment 8588658 [details] [diff] [review] still stands, but there's no telling whether we'll just crash somewhere else soon afterwards.

Comment 30

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

That might still be an improvement over a startup crash.

Comment 31

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> What's weird is I can't figure out why they'd be working around the block
> when it's just going to crash.

Maybe it's new malware?  comment 28 makes it sound like a different addon than what it was before.

> Changing the ordering of the IDL from bug 997325 might resolve this, but
> also might cause problems with other addons. It would be pretty risk.

Seems pretty plausible, though; the method we'd be calling without bug 997325 applied would be getMethodInfo...wonder if the second arg register is a valid pointer in these crashes.  Why do you say that moving the new isMainProcessScriptableOnly() method would be pretty risky?

Comment 32

[Sylvestre Ledru [:Sylvestre]]

We could just take David's patch, build an rc2 and publish it to see what happens...
As Liz said, that is better than nothing.

Comment 33

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

Yes, I like that idea. David, let's try landing your original attachment if you think it might lead us to more information or mitigating the startup crash somewhat. 

Nathan, my thought is that if it were new malware, we would be seeing this crash also spike on earlier betas.  But we aren't. It is only showing up in significant numbers for beta 7. 

According to this,
https://crash-analysis.mozilla.com/rkaiser/2015-06-22/2015-06-22.buildcrashes.html#Firefox-beta
ADI for beta 6 is at over 500K and beta 7 is at 1.3M. 
From crash-stats, we have 111 crashes total for beta 6 and 66201 for beta 7.
   
That's why it seems to me that it is more likely than not to be a regression from the uplifts between beta 6 and 7:  http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=cf82a529f8e9&tochange=900c78b9be48

Comment 34

[Nathan Froyd [:froydnj]]

None of those uplifts look to be related to the crashes, though.  The things on the stack are all related to DOM-y/docshell-y stuff; very few of those uplifts touch anything in DOM or docshell, and the things that touch DOM don't look like they some anywhere near the stuff on the stack.

I would be OK with applying/uplifting David's original patch, but at this late stage, I guess we're not really sure whether things wouldn't crash someplace else...should have gone with the simpler patch in the first place.  My mistake. :(

Comment 35

[(Away)]

I'm happy to give the null-check a try. What's the backup plan if it doesn't work?

Comment 36

[Nathan Froyd [:froydnj]]

Comment on attachment 8588658 [details] [diff] [review]
bandaid

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

I have no ideas for a backup plan.  I can't imagine what the addon will be expected by passing in a null pointer...

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98a929f58730

Comment 38

[(Away)]

Comment on attachment 8588658 [details] [diff] [review]
bandaid

Approval Request Comment
[Feature/regressing bug #]: external binary
[User impact if declined]: high-volume startup crashes
[Describe test coverage new/current, TreeHerder]: zero
[Risks and why]: Our main risk at this point is that this patch won't be enough to stop the crashes
[String/UUID change made/needed]: none

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8588658 [details] [diff] [review]
bandaid

Taking it in aurora for testing (even if I don't have much hope here).
and updating the flag.

Comment 40

[(Away)]

On the theory that they were actually attempting to call GetMethodInfo before the vtable change, I kinda worry that their code might look like:

for(i = 0, ...) {
  foo->GetMethodInfo(i, &ptr);
  ...
}

In which case the null check only helps us on the first iteration, and we'd crash with count==1 on the next iteration. Maybe we could do a try/catch around the write to the pointer? I also worry that they might not check the return value and use an uninitialized "ptr" parameter.

I'd be curious to hear Benjamin's specific concerns about reverting the vtable order. Yeah, it sucks to change a vtable and UUID this late, but we're in a pretty bad spot here.

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8588658 [details] [diff] [review]
bandaid

Let's take it to release to be able to have treeherder results.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/f075b15b50d5

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cfc6f3c1c1f6

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1e9835314fe8

Comment 45

[Andrei Vaida [:avaida]]

David, is there anything Manual QA can do here to help? My understanding is that this crash affects only setups that contain specific DLLs so it'll be pretty tough to reproduce the crash on our end without a sample. Either way, if you think there's something we should be looking at here, please let me know.

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98a929f58730

Comment 47

[(Away)]

(In reply to Andrei Vaida, QA [:avaida] from comment #45)
It would be very helpful if you could obtain a sample of the "Pay-By-Ads" addon. I don't know where to find it though. Maybe it comes bundled with some freeware?

Comment 48

[(Away)]

> On the theory that they were actually attempting to call GetMethodInfo
> before the vtable change, I kinda worry that their code might look like:
> 
> for(i = 0, ...) {
>   foo->GetMethodInfo(i, &ptr);
>   ...
> }
> 
> In which case the null check only helps us on the first iteration, and we'd
> crash with count==1 on the next iteration.

And that's exactly what happened.
https://crash-stats.mozilla.com/report/index/d618976c-d905-42ba-8356-ec1642150625
https://crash-stats.mozilla.com/report/index/288ffaee-ac55-40bf-b32f-c7f732150625

Comment 49

[(Away)]

Attachment: Check callers

Comment 50

[Nathan Froyd [:froydnj]]

Comment on attachment 8626415 [details] [diff] [review]
Check callers

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

Yuck.  But what else are you going to do?

I still think it might be worth flipping the IDL and trying that first.  Flipping the IDL would fix the topcrash, guaranteed, but open us up to some unknown (but we think it's likely to be fairly small) number of crashes.  This is going to stop things from crashing, but we don't know if it's going to cause problems later on down the line.

This is one of those times that you wish you could release to a small population of users at a time...

::: xpcom/reflect/xptinfo/xptiprivate.h
@@ +412,5 @@
>  private:
>      xptiInterfaceEntry* mEntry;
>      nsRefPtr<xptiInterfaceInfo> mParent;
> +#ifdef XP_WIN
> +    static void* BrokenModule;

I would really like it if this was called sBrokenModule.

Comment 51

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

Once more with feeling, let's land this on m-r.

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1089f3645fc

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/0b0822cabbb9

Comment 54

[(Away)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a894452f3e9

For the record, my preference is to try this patch first (vs reverting the IDL), because addons who are doing the right thing with the 39 SDK will not notice a difference. I worry that there are a nontrivial number of such addons, who would be broken if we revert the IDL.

The risk of the current approach is that it depends on the broken addon safely handling our errors. If the addons uses its (possibly-uninitialized) parameters despite our error codes, the crashes may continue. If that happens we can revert and try more desperate options.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4e2c643442d

Comment 56

[(Away)]

It didn't work :-( The crashes just moved elsewhere:

https://crash-stats.mozilla.com/search/?platform=Windows+NT&product=Firefox&version=39.0b&build_id=20150626112833&_facets=signature&_facets=build_id&_facets=version&_facets=release_channel&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

We'll need to back out and look at other options.

Comment 57

[Nathan Froyd [:froydnj]]

(In reply to David Major [:dmajor] from comment #56)
> It didn't work :-( The crashes just moved elsewhere:
> 
> https://crash-stats.mozilla.com/search/
> ?platform=Windows+NT&product=Firefox&version=39.
> 0b&build_id=20150626112833&_facets=signature&_facets=build_id&_facets=version
> &_facets=release_channel&_columns=date&_columns=signature&_columns=product&_c
> olumns=version&_columns=build_id&_columns=platform#crash-reports
> 
> We'll need to back out and look at other options.

Stupid question time: the crashes from this query ("signature facet" tab) look much more spread out and somewhat lower volume (?) than the startup crash.  For instance, the OOM signature looks like it already has several bugs associated with it, and I don't see why it would be correlated with these changes.  What motivates attributing the crashes from that link to the changes from this bug?

Comment 58

[(Away)]

> Stupid question time: the crashes from this query ("signature facet" tab)
> look much more spread out and somewhat lower volume (?) than the startup
> crash.  For instance, the OOM signature looks like it already has several
> bugs associated with it, and I don't see why it would be correlated with
> these changes.  What motivates attributing the crashes from that link to the
> changes from this bug?

It's more spread out because every machine now has a unique signature due to the random filenames, and there's a long tail. In total these crashes are in a similar ballpark as the xpti signature before, and it's still a startup crash.

Comment 59

[(Away)]

I'm still really scared of reverting the vtable order. But I'll cave if others really want to try it.

Could we live with this if it was a one-time startup crash rather than a permanent startup crash? What if, upon the first null pointer, we write the caller's filename into some temp file and block that on subsequent launches?

Any other ideas?

Comment 60

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1089f3645fc

Comment 61

[Robert Kaiser]

(In reply to David Major [:dmajor] from comment #59)
> Could we live with this if it was a one-time startup crash rather than a
> permanent startup crash?

I think we could. When Firefox crashes at a startup, I'm pretty sure the vast majority of people will try again. If it succeeds then, I think we're good.

Comment 62

[Nathan Froyd [:froydnj]]

Writing out the module name seems pretty reasonable.  Questions:

- Where is this file going to be stored?
- What happens if multiple places call into this with a null pointer?  Do we block all of them?
- Probably there are other things I've not thought of yet.

I still kind of want to revert the vtable change.

Comment 63

[(Away)]

> - Where is this file going to be stored?
> - What happens if multiple places call into this with a null pointer?  Do we
> block all of them?
> - Probably there are other things I've not thought of yet.

The blocklist is initialized before we have a profile, so we can't use the profile directory. I was originally thinking ::GetTempPath but that's kind of yucky. Perhaps we could use the crash reporter's pre-profile area (Users\...\Firefox\Crash Reports).

For now I'm envisioning an extremely targeted fix with just a single entry in the file, overwriting any previous versions of the file. At some point we may want to make it more general, but not for 39.

Comment 64

[Robert Kaiser]

(In reply to David Major [:dmajor] from comment #63)
> For now I'm envisioning an extremely targeted fix with just a single entry
> in the file, overwriting any previous versions of the file. At some point we
> may want to make it more general, but not for 39.

I fully agree with that. I've been of the opinion for a while that we should have the full DLL blocklist in an external file that could potentially be updated without a full Firefox update (of course we'd still need a mechanism for that which does not run inside the Firefox process). But let's take that to a followup.

Comment 65

[Nathan Froyd [:froydnj]]

So, current status:

- dmajor is currently en route home from #mozwww.  He has volunteered to write the patch described in comment 59.  This patch is probably pretty safe, unless there are *other* misbehaving binary addons that we're not aware of...?  I am also a little leery of giving outside programs the ability to block arbitrary DLLs in Firefox, but maybe that is not really a problem...

- I have written a patch that fixes the vtable ordering issues.  This patch would at least fix the crash that we're seeing, but might cause problems for *other* binary addons that did update to the new vtable order promised in beta1.  Or it might turn up other issues.  Not sure.

Decisions:

- We need to decide between them, or run testing on both of them, or...something else.
- We should probably backout dmajor's patch, since it didn't fix the problem, on all relevant branches:

https://hg.mozilla.org/mozilla-central/rev/a1089f3645fc
https://hg.mozilla.org/releases/mozilla-aurora/rev/b4e2c643442d
https://hg.mozilla.org/releases/mozilla-release/rev/0b0822cabbb9

Backing out the patch on central and aurora can be done straightaway; I guess we would want to wait on the m-r backout until we have another fix to land with it?

Liz, what next steps do release drivers want to take here?

Comment 66

[Nathan Froyd [:froydnj]]

Attachment: move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

dmajor asked me to attach this patch for comparison purposes.

Comment 67

[Robert Kaiser]

Oh, and given that the patch didn't work, this is not fixed.

Comment 68

[(Away)]

Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

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

::: xpcom/reflect/xptinfo/nsIInterfaceInfo.idl
@@ +28,1 @@
>  interface nsIInterfaceInfo : nsISupports

Should we also rev the UUID on anything that hands out an nsIInterfaceInfo?

Comment 69

[Ryan VanderMeulen [:RyanVM]]

Fx41: https://hg.mozilla.org/mozilla-central/rev/8758612313fa
Fx39: https://hg.mozilla.org/releases/mozilla-release/rev/a70e2969c711

Aurora/Beta are closed for the uplift right now. I'll do the Fx40 backout from Beta after the uplifts are done. Leaving the status flag set to fixed as a reminder.

Comment 70

[Jared Wein [:jaws] (please needinfo? me)]

Trying to see if I can be of any help here...

In beta 7 I uplifted a patch that conditionally launches the Windows 10 modern Settings application [1]. IApplicationActivationManager is available starting with Windows 8, but I'm not experienced enough with crash-stats to find a way to see a breakdown of crashes per Windows version.

Perhaps this has something to do with the crashes? Note that this patch is also present on mozilla-aurora and mozilla-central, and the crashes appear to be specific to beta only as far as I can tell. Most likely not the cause, but if people are looking for ideas of things to investigate...

[1] http://hg.mozilla.org/releases/mozilla-beta/rev/5e03a12dd57e#l3.32

Comment 71

[(Away)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #70)
The crashing machines have roughly our typical distribution of Windows versions. (If anything they have slightly more Windows 7 than usual, but not enough to make me suspicious)

From the symptoms that we're seeing, I'm convinced that the root cause is the vtable change in bug 997325. The spike in 39b7 is definitely strange, but at this point I'm not convinced that there is a causation.

Comment 72

[(Away)]

Attachment: Record the crashing module's filename and block it on subsequent launches

Here's the patch to record the crashing module name, if we want it.

I'll be the first to admit that there are some yucky aspects of this patch:
- There's a lot of buffer manipulation
- I had to stick with GetTempFileW to get the temp path. It would have been nicer to use the crash reporter's directory, but right now mozglue.dll doesn't know about that location, and I think it's too risky to plumb it through at this point.
- Normally we don't hardcode the word Firefox, but if we're going to stick a strange file into a user's %TEMP%, I'd like to be clear on where it's coming from to avoid surprise.

If we opt for this approach I'd like to get several reviewers, including those familiar with Windows APIs and the crash reporter.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

Fx40: https://hg.mozilla.org/releases/mozilla-beta/rev/8694405e0ce2

Comment 74

[(Away)]

Comment on attachment 8627352 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

I'm hoping at least two people can review this. Please look extra carefully at the buffer usage and Windows API calls.

Comment 75

[(Away)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=568f47238738

Comment 76

[Masatoshi Kimura [:emk]]

Comment on attachment 8627352 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +3402,5 @@
> +    return;
> +
> +  // Don't lock ourselves out by mistake
> +  if (wcsstr(moduleName, L"xul.dll"))
> +    return;

Is xul.dll the only file to be whitelisted?

Comment 77

[Masatoshi Kimura [:emk]]

How about *adding* the old interface with the old iid?

Comment 78

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

dmajor mentions mozglue.... could the change in bug 868814 have anything to do with the crash?

Comment 79

[Mike Hommey [:glandium]]

I don't see mozglue being mentioned as a cause of the problem. Merely as part of the technical details for the fix.

Comment 80

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8627352 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

General style nit: Curly braces around single line consequents of if statements

This looks good. My main concern is what :emk had to say about the whitelist. See my comment about dependentlibs.list. I'd either like to see another revision of this patch, or else convince me that my suggestion is unnecessary. :-)

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +600,5 @@
>  
> +  if (sPreviouslyCrashedModule[0] &&
> +      moduleFileName->Length < ArrayLength(sPreviouslyCrashedModule) &&
> +      !wcsicmp(sPreviouslyCrashedModule, moduleFileName->Buffer)) {
> +    printf_stderr("LdrLoadDll: Blocking load of previously crashed module '%s'\n", dllName);

Is this printf_stderr something you want to leave in? (I suppose I'm ok with it, just checking)

@@ +737,5 @@
> +                &nBytes,
> +                nullptr)) {
> +    sPreviouslyCrashedModule[0] = L'\0';
> +  }
> +

In the successful case:
sPreviouslyCrashedModule[nBytes] = L'\0';
since the saved filename does not include null terminator and to be consistent with the fact that you set it in the failed case.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +3402,5 @@
> +    return;
> +
> +  // Don't lock ourselves out by mistake
> +  if (wcsstr(moduleName, L"xul.dll"))
> +    return;

Ditto... it seems to me that the moduleName should not intersect with the set of libraries in dependentlibs.list

Comment 81

[Daniel Veditz [:dveditz]]

Comment on attachment 8627352 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +204,5 @@
>  static DWORD sThreadLoadingXPCOMModule;
>  static bool sBlocklistInitFailed;
>  static bool sUser32BeforeBlocklist;
>  
> +static wchar_t sPreviouslyCrashedModule[MAX_PATH];

Should this be MAX_PATH+1 for the null?

@@ +715,5 @@
> +  wchar_t tempPath[MAX_PATH];
> +  if (!GetTempPathW(ArrayLength(tempPath), tempPath))
> +    return;
> +
> +  if (wcscat_s(tempPath, ArrayLength(tempPath), L"FirefoxBlockedDLL.txt"))

Why ArrayLength() here and earlier but sizeof() later?

@@ +735,5 @@
> +                sPreviouslyCrashedModule,
> +                sizeof(sPreviouslyCrashedModule),
> +                &nBytes,
> +                nullptr)) {
> +    sPreviouslyCrashedModule[0] = L'\0';

There are a couple places this function can bail out without initializing sPreviouslyCrashedModule. Maybe move this to the top of the function and do it unconditionally.

Do we always null-terminate the contents we write to the file? What if someone malicious (such as the dlls we're trying to block) puts something really long in there or otherwise not null terminated? I guess it wouldn't be terrible -- the wcsicmp() should stop at the end of the module name and that will be less than the length of the array.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +3402,5 @@
> +    return;
> +
> +  // Don't lock ourselves out by mistake
> +  if (wcsstr(moduleName, L"xul.dll"))
> +    return;

xul.dll isn't the only potential problem. I hope this is a short-lived hack because this does raise the possibility of mysterious failures to launch that will be pretty hard to diagnose, and which won't be fixed by reinstalling Firefox because we'll use the same temp file.

Comment 82

[Daniel Veditz [:dveditz]]

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #80)
> In the successful case:
> sPreviouslyCrashedModule[nBytes] = L'\0';

but watch out for nBytes == sizeof(sPreviouslyCrashedModule)

Comment 83

[(Away)]

(In reply to Masatoshi Kimura [:emk] from comment #77)
> How about *adding* the old interface with the old iid?

Nathan and I discussed this last week. We think that the addon receives an nsIInterfaceInfo as an out-param, and uses it blindly without a QI, something like:
  nsIInterfaceInfo* ptr;
  foo->GetInterfaceInfo(&ptr);
  for (i = 0; ...)
    ptr->GetMethodInfo(i, ...); // goes to GetConstantCount

So responding to the old UUID wouldn't help there.

Comment 84

[(Away)]

> Is xul.dll the only file to be whitelisted?
 
> This looks good. My main concern is what :emk had to say about the
> whitelist. See my comment about dependentlibs.list. I'd either like to see
> another revision of this patch, or else convince me that my suggestion is
> unnecessary. :-)

It's intended more as a fail-safe than a whitelist. No code other than the broken addon should ever be passing a null pointer, so we should never see an internal binary in this codepath. I just want to protect against "_ReturnAddress()" somehow being incorrect and blaming libxul further up (for example we might lose the real return address in a tail call, though I admit it's extremely unlikely in this situation). Do you think the other dependentlibs are at risk of such things?

Comment 85

[Nicolas B. Pierron [:nbp]]

I modified the search based on the moved crashes (comment 52), and remarked that the same address appeared in multiple of these libraries.

The following link seems to highlight some of the crashes with the same stack:

https://crash-stats.mozilla.com/search/?platform=Windows+NT&product=Firefox&signature=~%400x1ad64&_facets=signature&_facets=build_id&_facets=version&_facets=release_channel&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Looking at the top 3, of these libraries:
https://www.herdprotect.com/xmnjnyfx.dll-b7383216e66711b6d3d951158ffe23a976c1d2f2.aspx
https://www.herdprotect.com/hhhlmpyl.dll-40dc2f3ebe59cab2f42837d63055d95670f7ee5c.aspx
https://www.herdprotect.com/ubpouego.dll-cd0d2b436e0b1170d3bed11ce4150b3b684eefd2.aspx

we can notice that they have identical PE Metadata.
Maybe, this would be enough to filter out these libraries.

On the other hand, these are not the same as the one reported in comment 4.

Comment 86

[(Away)]

(In reply to Nicolas B. Pierron [:nbp] from comment #85)

> we can notice that they have identical PE Metadata.
> Maybe, this would be enough to filter out these libraries.
> 
> On the other hand, these are not the same as the one reported in comment 4.

Indeed. Many of the modules have that same June timestamp, but I've also seen other variants. And it leaves us at risk of them shipping an update that still uses the old SDK.

Comment 87

[(Away)]

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #80)
> General style nit: Curly braces around single line consequents of if
> statements

Will do.

> This looks good. My main concern is what :emk had to say about the
> whitelist. See my comment about dependentlibs.list. I'd either like to see
> another revision of this patch, or else convince me that my suggestion is
> unnecessary. :-)

Per comment 84 I'm still mostly worried about libxul, but if it makes you feel better, what if we delete the temp file if we reach the "Couldn't load XPCOM" error path?

> Is this printf_stderr something you want to leave in? (I suppose I'm ok with
> it, just checking)

printf_stderr is pretty common in that function. And I'm kind of hoping to get the vendor's attention that they're crashing.
 
> In the successful case:
> sPreviouslyCrashedModule[nBytes] = L'\0';
> since the saved filename does not include null terminator and to be
> consistent with the fact that you set it in the failed case.

Ok (modulo comment 82)

Comment 88

[(Away)]

(In reply to Daniel Veditz [:dveditz] from comment #81)
> > +static wchar_t sPreviouslyCrashedModule[MAX_PATH];
> 
> Should this be MAX_PATH+1 for the null?

It wouldn't hurt anything.

> > +  if (wcscat_s(tempPath, ArrayLength(tempPath), L"FirefoxBlockedDLL.txt"))
> 
> Why ArrayLength() here and earlier but sizeof() later?

wcscat_s takes a number of wchar_t elements. WriteFile knows nothing about types so it speaks in bytes. Actually I could probably use the templated variant of wcscat to auto-infer the length.

> @@ +735,5 @@
> > +                sPreviouslyCrashedModule,
> > +                sizeof(sPreviouslyCrashedModule),
> > +                &nBytes,
> > +                nullptr)) {
> > +    sPreviouslyCrashedModule[0] = L'\0';
> 
> There are a couple places this function can bail out without initializing
> sPreviouslyCrashedModule. Maybe move this to the top of the function and do
> it unconditionally.

As a global it ought to be zeroed already, but I'm not opposed to adding an initialization anyway.

> Do we always null-terminate the contents we write to the file? What if
> someone malicious (such as the dlls we're trying to block) puts something
> really long in there or otherwise not null terminated? I guess it wouldn't
> be terrible -- the wcsicmp() should stop at the end of the module name and
> that will be less than the length of the array.

Indeed; that was my reason for adding the moduleFilename->Length check.

Comment 89

[Vasilica Tamas, QA [:vtamas]]

We performed exploratory testing around the latest versions of antivirus-related add-ons across Windows 7 64-bit on:
  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-de83156d7de9/try-win32/
  http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-568f47238738/try-win32/

For now, we are concerned about the following result:
- Norton Toolbar 2015.2.1.5 is automatically disabled despite the fact that xpinstall.signatures.required pref is set to false (This happened before Norton account was created) → http://i.imgur.com/ErXild7.png
- After Norton account was created and the install process was finished, we restart the Firefox -> An Error Message appeared: Couldn’t load XPCOM → http://i.imgur.com/TxbOnN7.jpg
- We tried to open Firefox from application, but another error message was displayed: Could not find the Mozilla runtime → http://i.imgur.com/sbA7hAT.jpg  

This behavior is not reproducible on Firefox 39 RC build 5 because the Norton Toolbar 2015.2.1.5 is not compatible with Firefox 39 → http://i.imgur.com/vC4oFaq.jpg

We have also installed several popular add-ons from addons.mozilla.org and no new issues were encountered. 

We will investigate further this issue. Our testing results are tracked in https://etherpad.mozilla.org/bug1151506

Comment 90

[(Away)]

Thank you Vasilica! These are important findings. Did the errors appear with both of the try-builds, or just one?

Comment 91

[Ada [:adalucinet]]

(In reply to David Major [:dmajor] from comment #90)
> Thank you Vasilica! These are important findings. Did the errors appear with
> both of the try-builds, or just one?

The errors are visible for both try-builds.

Comment 92

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Major [:dmajor] from comment #84)
> > Is xul.dll the only file to be whitelisted?
>  
> > This looks good. My main concern is what :emk had to say about the
> > whitelist. See my comment about dependentlibs.list. I'd either like to see
> > another revision of this patch, or else convince me that my suggestion is
> > unnecessary. :-)
> 
> It's intended more as a fail-safe than a whitelist. No code other than the
> broken addon should ever be passing a null pointer, so we should never see
> an internal binary in this codepath. I just want to protect against
> "_ReturnAddress()" somehow being incorrect and blaming libxul further up
> (for example we might lose the real return address in a tail call, though I
> admit it's extremely unlikely in this situation). Do you think the other
> dependentlibs are at risk of such things?

I see what you're saying. You're right, this is very unlikely to begin with. I think that it is completely possible for those other libs to be somewhere on the call stack, though you'd see libxul above the others. OTOH this could probably get really nutty if we start thinking of the full graph of dependent libraries. We need to draw a line somewhere.

Comment 93

[(Away)]

> The errors are visible for both try-builds.

Hmm, that's strange. Do these errors happen on a regular Nightly?

Comment 94

[(Away)]

Attachment: Record the crashing module's filename and block it on subsequent launches

- Removed xul.dll check in favor of DeleteFile
- Added extra (perhaps excessive) safety on sPreviouslyCrashedModule
- Added curly braces
- I decided to stick with wcscat_s because the size-templated wcscat does not check for sufficient space in the buffer

Comment 95

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8627773 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

LGTM

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +712,5 @@
>  
> +void ReadPreviouslyCrashedModule()
> +{
> +  memset(sPreviouslyCrashedModule, 0, sizeof(sPreviouslyCrashedModule));
> +  

Nit: clear out the spaces from this blank line

::: xpcom/reflect/xptinfo/xptiInterfaceInfo.cpp
@@ +778,5 @@
> +    }
> +
> +    if (!aConstantCount) {
> +#if defined(XP_WIN) && defined(MOZ_CRASHREPORTER)
> +        CrashReporter::RecordCrashingModule(_ReturnAddress());

Does this need to include intrin.h or anything for _ReturnAddress()? Meh, I guess as long as it builds...

Comment 96

[Benjamin Smedberg]

Comment on attachment 8627773 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

It's unfortunate that this file has to be in the tempdir. With additional plumbing we could probably store this in AppData/Roaming/Mozilla/Firefox, but that's probably too complex for the timeframe at hand.

Is the intention to remove this code in a cycle or two? I bet we could do better in a new release by de-XPCOMing all of the interfaceinfo stuff.

Comment 97

[(Away)]

Comment on attachment 8627773 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +598,5 @@
>      }
>    }
>  
> +  if (sPreviouslyCrashedModule[0] &&
> +      moduleFileName->Length < ArrayLength(sPreviouslyCrashedModule) &&

Per docs (and verified in debugger) moduleFileName->Length is measured in bytes. I need to use sizeof here.

Comment 98

[(Away)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #96)
> It's unfortunate that this file has to be in the tempdir. With additional
> plumbing we could probably store this in AppData/Roaming/Mozilla/Firefox,
> but that's probably too complex for the timeframe at hand.

Yeah, getting that right is a risk that I don't want to take this late.
 
> Is the intention to remove this code in a cycle or two? I bet we could do
> better in a new release by de-XPCOMing all of the interfaceinfo stuff.

I would love to rip this out as soon as we can get away with it. I'll file a follow-up bug if this patch sticks.

Comment 99

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(Aaron, Dmajor,

User Advocacy is preparing an infection control compaign to contact affected users.  We need guidance on what data you need / want from those users, and how to get it.  Happy to vidyo, email, or chat.

)

Comment 100

[Daniel Veditz [:dveditz]]

Comment on attachment 8627773 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

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

Looks good except for the null-termination math. Normally I'd "r+ with nits" but since we're in a stressful last-minute situation it will be clearer for everyone (especially sheriffs doing branch transplants) to have a fresh patch.

::: mozglue/build/WindowsDllBlocklist.cpp
@@ +737,5 @@
> +
> +  DWORD nBytes = 0;
> +  if (ReadFile(inputFile,
> +               sPreviouslyCrashedModule,
> +               sizeof(sPreviouslyCrashedModule) - 1,

minus 2 or sizeof(wchar_t) ?  If you round down when null terminating (nBytes/2 below) it's not terrible: we might stomp a byte we read, but it'd be an illegal half character anyway.

@@ +740,5 @@
> +               sPreviouslyCrashedModule,
> +               sizeof(sPreviouslyCrashedModule) - 1,
> +               &nBytes,
> +               nullptr)) {
> +    sPreviouslyCrashedModule[nBytes] = L'\0';

nBytes is bytes but the array is wchar_t -- this could be stomping way out in space and in any case won't be where we want it. On the other hand as long as the file name fills less than half the array the overly paranoid memset() initialization will make sure it's correctly null terminated.

You could sanity check the data -- if it's odd we didn't write it
   if (nBytes % 2)  <recover>

Definitely need to put the null at nBytes/2 though.

Comment 101

[(Away)]

Attachment: Record the crashing module's filename and block it on subsequent launches

Carrying r=aklotz, and got r=dveditz by IRC. Thanks for catching that.

Approval Request Comment
[Feature/regressing bug #]: external app and bug 997325
[User impact if declined]: high-volume startup crashes
[Describe test coverage new/current, TreeHerder]: local testing under debugger
[Risks and why]: As before, we have the risk that this doesn't work. Also it's rather inelegant that we have to touch the temp directory. We'll especially need to watch how AV software reacts to this code.
[String/UUID change made/needed]: none

Comment 102

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

Comment on attachment 8627853 [details] [diff] [review]
Record the crashing module's filename and block it on subsequent launches

Approved to land on m-r.

Comment 103

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/a3f685acd61a

Comment 104

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

This is building now and I would like to ship it to the beta channel in the morning. 

Florin, Vasilica, Alexandra (and anyone else who cares to test), it will probably be good to give this new build a try on Windows with antivirus software installed. I don't think we should test much more than that, but anything you can do will surely be helpful.

Comment 105

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

For anyone else who wants to test this, you will be able to download build 6 from this directory: ftp://ftp.mozilla.org/pub/firefox/candidates/39.0-candidates/    I believe we especially want testing on Windows.

Comment 106

[Ada [:adalucinet]]

(In reply to David Major [:dmajor] from comment #93)
> > The errors are visible for both try-builds.
> 
> Hmm, that's strange. Do these errors happen on a regular Nightly?

No, not all of them - with latest 42.0a1 (from 2015-06-30) *only* the fact that Norton Toolbar 2015.2.1.5 is automatically disabled (screenshot: http://i.imgur.com/ErXild7.png), with or without an Norton account. With 39.0 RC build 6 (Build ID: 20150630154324), under 2 different Windows 7 64-bit machines and Windows 8.1 32-bit, Norton toolbar is incompatible -> http://i.imgur.com/IKwlq0o.png
Apparently, the "An Error Message appeared: Couldn’t load XPCOM" and "Could not find the Mozilla runtime" error messages are *only* applicable for the try-builds.

Please note that this testing is on-going and we will update here as soon as we are done.

Comment 107

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Testing was performed today with 39 RC build 6, covering Add-ons (AntiVirus and other more popular add-ons) and Web Compatibility (major websites with AV add-ons enabled). 

Testing went fine with no issues related to this fix.

Testing covered: Win 7 x64 (2 machine), Win 8.1 x86, Mac OS X 10.10.4, Ubuntu 14.04 x86. More testing details and complete results can be found at: https://etherpad.mozilla.org/Fx39RC6.

Comment 108

[Matt Powers]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #106)
> (In reply to David Major [:dmajor] from comment #93)
> > > The errors are visible for both try-builds.
> > 
> > Hmm, that's strange. Do these errors happen on a regular Nightly?
> 
> No, not all of them - with latest 42.0a1 (from 2015-06-30) *only* the fact
> that Norton Toolbar 2015.2.1.5 is automatically disabled (screenshot:
> http://i.imgur.com/ErXild7.png), with or without an Norton account. With
> 39.0 RC build 6 (Build ID: 20150630154324), under 2 different Windows 7
> 64-bit machines and Windows 8.1 32-bit, Norton toolbar is incompatible ->
> http://i.imgur.com/IKwlq0o.png
> Apparently, the "An Error Message appeared: Couldn’t load XPCOM" and "Could
> not find the Mozilla runtime" error messages are *only* applicable for the
> try-builds.
> 
> Please note that this testing is on-going and we will update here as soon as
> we are done.

FF39-compatible extension for Norton Toolbar comes only with our 22.5+ versions of v22 products. (also released for our v21 and v20 products).

Here is a download link to Norton Security trial for the 22.5 version you should try in place of the current N360 trial link in your exploratory testing:

http://buy-download.norton.com/downloads/2015/22.5/NS/US/NS-ESD-22.5.0.124-EN.exe

Live Update must be run after install to apply a patch for FF39-compatible extension.

Comment 109

[Matt Powers]

(In reply to Matt Powers from comment #108)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #106)
> > (In reply to David Major [:dmajor] from comment #93)
> > > > The errors are visible for both try-builds.
> > > 
> > > Hmm, that's strange. Do these errors happen on a regular Nightly?
> > 
> > No, not all of them - with latest 42.0a1 (from 2015-06-30) *only* the fact
> > that Norton Toolbar 2015.2.1.5 is automatically disabled (screenshot:
> > http://i.imgur.com/ErXild7.png), with or without an Norton account. With
> > 39.0 RC build 6 (Build ID: 20150630154324), under 2 different Windows 7
> > 64-bit machines and Windows 8.1 32-bit, Norton toolbar is incompatible ->
> > http://i.imgur.com/IKwlq0o.png
> > Apparently, the "An Error Message appeared: Couldn’t load XPCOM" and "Could
> > not find the Mozilla runtime" error messages are *only* applicable for the
> > try-builds.
> > 
> > Please note that this testing is on-going and we will update here as soon as
> > we are done.
> 
> FF39-compatible extension for Norton Toolbar comes only with our 22.5+
> versions of v22 products. (also released for our v21 and v20 products).
> 
> Here is a download link to Norton Security trial for the 22.5 version you
> should try in place of the current N360 trial link in your exploratory
> testing:
> 
> http://buy-download.norton.com/downloads/2015/22.5/NS/US/NS-ESD-22.5.0.124-
> EN.exe
> 
> Live Update must be run after install to apply a patch for FF39-compatible
> extension.

My apologies, the download link above does not provide a trial product, but rather one that must be immediately activated with a product key (to allow access to the UI to let you run LiveUpdate).

I will find an alternate URL for a real trial-download.

Comment 110

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

I think we should do this for 40/41/42 (and land it in time for 40 beta 1!).  If Benjamin is around it would be nice to know if he agrees with that plan.

Comment 111

[(Away)]

Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

Per discussion we'd like to land this on beta before 40b1 ships. It would allow us to keep the temp-file bandaid confined to version 39. If beta1 ships without this, then we won't be able to change the UUID until 41.

Approval Request Comment
[Feature/regressing bug #]: external app and bug 997325
[User impact if declined]: We'll have to keep the v39 hack in v40
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: This is low risk as long as it goes out with the first beta. Otherwise we'll have to wait another cycle.
[String/UUID change made/needed]: Yes /!\

Comment 112

[(Away)]

Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

See comment 111

Comment 113

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

Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

Approving for uplift to Beta as this is a startup crash that affects a significant set of end-users.

Comment 114

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

OK. This also means we risk breaking some antivirus addons.  Since this puts things back to the vtable load order from March.

We don't know that it breaks anything, but we should test and react to that in early beta.

Comment 115

[(Away)]

We expect that the vendors who are doing the right thing will recompile with the new SDK at beta 1 like always. Those who aren't, presumably didn't recompile for 39, so we'll be putting them back to what they're used to.

Comment 116

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

https://hg.mozilla.org/releases/mozilla-beta/rev/0e0e1e0151cb

Comment 117

[Matt Powers]

(In reply to Matt Powers from comment #109)
> (In reply to Matt Powers from comment #108)
> > (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #106)
> > > (In reply to David Major [:dmajor] from comment #93)
> > > > > The errors are visible for both try-builds.
> > > > 
> > > > Hmm, that's strange. Do these errors happen on a regular Nightly?
> > > 
> > > No, not all of them - with latest 42.0a1 (from 2015-06-30) *only* the fact
> > > that Norton Toolbar 2015.2.1.5 is automatically disabled (screenshot:
> > > http://i.imgur.com/ErXild7.png), with or without an Norton account. With
> > > 39.0 RC build 6 (Build ID: 20150630154324), under 2 different Windows 7
> > > 64-bit machines and Windows 8.1 32-bit, Norton toolbar is incompatible ->
> > > http://i.imgur.com/IKwlq0o.png
> > > Apparently, the "An Error Message appeared: Couldn’t load XPCOM" and "Could
> > > not find the Mozilla runtime" error messages are *only* applicable for the
> > > try-builds.
> > > 
> > > Please note that this testing is on-going and we will update here as soon as
> > > we are done.
> > 
> > FF39-compatible extension for Norton Toolbar comes only with our 22.5+
> > versions of v22 products. (also released for our v21 and v20 products).
> > 
> > Here is a download link to Norton Security trial for the 22.5 version you
> > should try in place of the current N360 trial link in your exploratory
> > testing:
> > 
> > http://buy-download.norton.com/downloads/2015/22.5/NS/US/NS-ESD-22.5.0.124-
> > EN.exe
> > 
> > Live Update must be run after install to apply a patch for FF39-compatible
> > extension.
> 
> My apologies, the download link above does not provide a trial product, but
> rather one that must be immediately activated with a product key (to allow
> access to the UI to let you run LiveUpdate).
> 
> I will find an alternate URL for a real trial-download.

Here is a working trialware download URL:

http://us.norton.com/norton-security-downloads-trial?lg=en&ct=US

You will need to create or supply an existing Norton Account.  

Then run LiveUpdate (under the Security section of the main UI) to apply patches - one of which is FF39 compatible Norton Toolbar.

Then start FF39 and if needed enable the extension.

Comment 118

[Masatoshi Kimura [:emk]]

(In reply to Liz Henry (:lizzard) from comment #114)
> OK. This also means we risk breaking some antivirus addons.  Since this puts
> things back to the vtable load order from March.

Does it matter? I heard Firefox 40+ dropped the support for binary XPCOM components in addons.

Comment 119

[Vladan Djeric (:vladan)]

I found an installer for the offending adware on VirusTotal. Does anyone have a Windows VM handy to test it?

Comment 120

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

(In reply to Masatoshi Kimura [:emk] from comment #118)
> Does it matter? I heard Firefox 40+ dropped the support for binary XPCOM
> components in addons.

It'll probably be pushed back to 41, per bug 1179902.

Comment 121

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #119)
> I found an installer for the offending adware on VirusTotal. Does anyone
> have a Windows VM handy to test it?

Vladan, do you still need help testing this? After a discussion with our IT people it seems we could safely test this on a VM here, so if you still need help you can send me the installer and we'll give it a try.

Comment 122

[Vladan Djeric (:vladan)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #121)
> Vladan, do you still need help testing this? After a discussion with our IT
> people it seems we could safely test this on a VM here, so if you still need
> help you can send me the installer and we'll give it a try.

Aaron Klotz tried it on a VM, I tried it without a VM. Sadly, launching the installer EXE was not enough to get the malware to infect the bait machines.

You can give it another shot if you like -- I'll forward you the EXE.

Comment 123

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #122)
> Aaron Klotz tried it on a VM, I tried it without a VM. Sadly, launching the
> installer EXE was not enough to get the malware to infect the bait machines.
> 
> You can give it another shot if you like -- I'll forward you the EXE.

I gave it a try yesterday and today, but same as you guys I also had no luck reproducing the crash.

Comment 124

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d278252858a

Comment 125

[Nathan Froyd [:froydnj]]

Since this fix seems to be happy on beta, dmajor asked me to push it to inbound.  Will be nomming for Aurora next.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d278252858a

Comment 126

[Nathan Froyd [:froydnj]]

Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

Approval Request Comment
[Feature/regressing bug #]: bug 997325
[User impact if declined]: Crashes from malware binary addons/DLLs
[Describe test coverage new/current, TreeHerder]: The same change was made to our initial beta releases and seems to have worked out OK.
[Risks and why]: Low risk.
[String/UUID change made/needed]: One UUID change to nsIInterfaceInfo.  Shouldn't be a problem for Aurora.

Comment 127

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d278252858a

Comment 128

[Kate Glazko]

Comment on attachment 8627232 [details] [diff] [review]
move nsIInterfaceInfo::isMainProcessScriptable to the end of the interface's vtable

Already has been on Beta and m-c, closing the loop on Aurora.

Comment 129

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3be15f4b648