523915 - updater crashes when trying to update with crash reporter open

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[(not currently active) Ted Mielczarek]

Attachment: stack

I crashed, then restarted from the crash reporter, and the updater launched to apply a pending update. It then crashed with the attached stack, presumably because the crash reporter was still open. We clearly need to make this work right. I'm pretty sure it didn't used to crash, it would just fail to apply the update.

Comment 1

[Justin Dolske [:Dolske]]

So, the immediate cause of the crash is that we're passing a null pointer to fclose() and it doesn't like that; ensure_open() doesn't seen to have error handling for the open fails. We should fix that to avoid the crash.

Things actually start going wrong earlier, though..

updater.exe!backup_finish(const wchar_t * path=0x00000000, int status=0x00000007)

We're somehow getting a null pointer for the path here.

Can you attach the update.log associated with the run that crashed? I'm curious how it got into this state.

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: last-update.log

This is the last-update.log I currently have. Unfortunately it's truncated because of the crash. :-/

I think you can reproduce this by downloading an update, crashing, then restarting from the crash reporter so the updater starts while the crash reporter is still running.

Comment 4

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

Attachment: patch

Simple fix... surprised this wasn't found a long time ago.

Comment 5

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

Comment on attachment 412123 [details] [diff] [review]
patch

Actually, this should cleanup after itself as well

Comment 6

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

Attachment: patch rev2

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to comment #4)
> Created an attachment (id=412123) [details]
> patch
> 
> Simple fix... surprised this wasn't found a long time ago.

xref bug 498611

Comment 8

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

Attachment: patch rev3 - now with a little cleanup

Comment 9

[Justin Dolske [:Dolske]]

(In reply to comment #1)

> updater.exe!backup_finish(const wchar_t * path=0x00000000, int
> status=0x00000007)
> 
> We're somehow getting a null pointer for the path here.

I wonder if the function args are lies in this stack... The frame right above this is PatchFile::Finish, and that code is:

1016 void
1017 PatchFile::Finish(int status)
1018 {
1019   LOG(("FINISH PATCH " LOG_S "\n", mDestFile));
1020 
1021   backup_finish(mDestFile, status);
1022 }

If mDestFile was really null, it should have crashed in the LOG().

Comment 10

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

(In reply to comment #7)
>...
> xref bug 498611
For that either crashreporter is going to have to be copied before launching or bug 466778 will need to be fixed.

Comment 11

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

(In reply to comment #9)
> (In reply to comment #1)
> 
> > updater.exe!backup_finish(const wchar_t * path=0x00000000, int
> > status=0x00000007)
> > 
> > We're somehow getting a null pointer for the path here.
> 
> I wonder if the function args are lies in this stack... The frame right above
> this is PatchFile::Finish, and that code is:
> 
> 1016 void
> 1017 PatchFile::Finish(int status)
> 1018 {
> 1019   LOG(("FINISH PATCH " LOG_S "\n", mDestFile));
> 1020 
> 1021   backup_finish(mDestFile, status);
> 1022 }
> 
> If mDestFile was really null, it should have crashed in the LOG().
Yep... it was crashing in ensure_open when calling fclose after _wfopen (NS_tfopen) was unable to open the file

Comment 12

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

Comment on attachment 412127 [details] [diff] [review]
patch rev3 - now with a little cleanup

>diff --git a/toolkit/mozapps/update/src/updater/updater.cpp b/toolkit/mozapps/update/src/updater/updater.cpp
>--- a/toolkit/mozapps/update/src/updater/updater.cpp
>+++ b/toolkit/mozapps/update/src/updater/updater.cpp
>@@ -469,26 +469,25 @@ static int ensure_remove(const NS_tchar 
>   if (rv)
>     LOG(("remove failed: %d,%d (" LOG_S ")\n", rv, errno, path));
>   return rv;
> }
> 
> static FILE* ensure_open(const NS_tchar *path, const NS_tchar* flags, unsigned int options)
> {
>   ensure_write_permissions(path);
>+
>+  if (NS_tchmod(path, options) != 0)
>+    return NULL;
>+
>+  struct stat ss;
>+  if (NS_tstat(path, &ss) != 0 || ss.st_mode != options)
>+    return NULL;
>+
>   FILE* f = NS_tfopen(path, flags);
>-  if (NS_tchmod(path, options) != 0) {
>-    fclose(f);
>-    return NULL;
>-  }
>-  struct stat ss;
>-  if (NS_tstat(path, &ss) != 0 || ss.st_mode != options) {
>-    fclose(f);
>-    return NULL;
>-  }
>   return f;
bah... this should just be 
return NS_tfopen(path, flags);

Comment 13

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

Comment on attachment 412127 [details] [diff] [review]
patch rev3 - now with a little cleanup

It seems that something in my patch for bug 525390 broke this patch... will resubmit after I figure it out

Comment 14

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

Attachment: patch rev4

all better. The reason for the additional changes is the updated style guide.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 412125 [details] [diff] [review]
patch rev2

Does this mean we'll just fail to apply the update instead of crashing?

Comment 16

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

Yep

Comment 17

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

Keep in mind that "fail to apply" includes relaunching the app and the normal warning that the update could not be applied when a file is in use which allows retrying.

Comment 18

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

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

Comment 19

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

Comment on attachment 412177 [details] [diff] [review]
patch rev4

Drivers, this fixes a crash in the updater that breaks updating badly.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 412177 [details] [diff] [review]
patch rev4

a192=beltzner

Comment 21

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

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

Comment 22

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

To verify with a nightly 1.9.2 or trunk when you have an update ready to apply:
1. launch the crashreporter and leave the dialog open
2. apply the update.
3. an error message should be displayed regarding closing other instances of the app after applying the update has failed.
4. there shouldn't be a copy of the crashreporter.exe with .moz-backup in the app dir.
5. you should be able to close the crashreporter dialog and apply the update successfully without having to download it again.

Comment 23

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

btw: I verified this with today's nightly partial