588163 - Crash [@ @0x0 | ProcessUpdates(nsIFile*, nsIFile*, nsIFile*, int, char**, char const*&) ]

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

There has been an increase of crashes in ProcessUpdates on 1.9.2

http://tinyurl.com/2f7rnzu

bsmedberg analyzed the crash

     00000000()   
>    xul.dll!ScanDir(dir=0x00716520, result=0x0013fc7c)  Line 198    C++
     xul.dll!ProcessUpdates(greDir=0x007163a0, appDir=0x00716280, updRootDir=0x00000000, argc=0x00000001, argv=0x007290d8, appVersion=0x00717320)     C++
     xul.dll!XRE_main(argc=0x00000001, argv=0x007290a8, aAppData=0x007132c0) Line 3170    C++

Reading the assembly, it looks like "dir" passed to ScanDir is invalid (has a NULL vtable). That's quite strange and points to possible memory corruption.

static nsresult
ScanDir(nsIFile *dir, nsCOMArray<nsIFile> *result)
{
1063383A  push        ebp
1063383B  mov         ebp,esp
1063383D  sub         esp,14h
10633840  push        esi
10633841  mov         esi,dword ptr [ebp+8]
10633844  push        edi
  nsresult rv;

  nsCOMPtr<nsISimpleEnumerator> simpEnum;
10633845  lea         ecx,[simpEnum]
10633848  call        nsCOMPtr<nsIRDFNode>::nsCOMPtr<nsIRDFNode> (10469308h)
  rv = dir->GetDirectoryEntries(getter_AddRefs(simpEnum));
1063384D  lea         eax,[simpEnum]
10633850  push        eax
10633851  lea         eax,[ebp-10h]
10633854  push        eax
10633855  call        getter_AddRefs<nsICollation> (10415ABDh)
1063385A  pop         ecx
1063385B  pop         ecx
1063385C  mov         ecx,dword ptr [eax]
1063385E  call        nsCOMPtr<nsIWeakReference>::StartAssignment (1022C110h)
10633863  mov         ecx,dword ptr [esi]  <-- crash is here, esi is NULL, this should be *dir to get the vtable address. Because the minidump doesn't have heap memory, I don't know what *dir actually is.
10633865  push        eax
10633866  push        esi
10633867  call        dword ptr [ecx+0BCh]

The other possibility is that one of the intervening functions is tromping on ESI (which is supposed to be a preserved register in x86, but...).

No obvious clues. 

It is trivial to remove the call to ScanDir since we always require the update to be in the '0' sub-directory. This *might* fix the crash and definitely shouldn't cause any additional problems.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch - always use the '0' dir for updates

Comment 2

[Robert Strong (they/them - no direct email)]

Note: I wouldn't be at all adverse to having this on trunk as well since it isn't clear what it was trying to accomplish or that we would every want to implement whatever it was trying to implement

Comment 3

[Robert Strong (they/them - no direct email)]

For reference, the directory under updates has always had to be the 0 directory.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#419

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 466785 [details] [diff] [review]
patch - always use the '0' dir for updates

Drivers, this is mainly code removal which attempts to fix the increase in crashes we've seen with Firefox 3.6.9pre. This greatly simplifies the code by requiring the directory that contains the update to be in the '0' directory which has always been a requirement anyway. Since the ability to have multiple update patches has never been implemented I am landing this patch on trunk as well since the ability to have multiple patches has never been implemented.

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch for landing on mozilla-central

Comment 6

[LegNeato]

Comment on attachment 466785 [details] [diff] [review]
patch - always use the '0' dir for updates

a=LegNeato for 1.9.2.9. Please land this on mozilla-1.9.2 default as soon as possible.

Comment 7

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cb079fb7300a

Comment 8

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/9480e51be5b7

Comment 9

[Robert Strong (they/them - no direct email)]

For reference:

After looking at the query on crash-stats it seems that all of the crashes using 3.6.9pre occurred on 8/14 and the users had been using an 8/13 build. I suspect that the patch I landed (still a good thing to do) might not actually fix this bug and that the cause would have had to be something that landed after the 8/13 build and before the 8/14 build. I looked at the range and nothing at all looked like it might have been the cause regretfully.