Closed Bug 523915 Opened 10 years ago Closed 10 years ago

updater crashes when trying to update with crash reporter open

Categories

(Toolkit :: Application Update, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta3-fixed

People

(Reporter: ted, Assigned: rstrong)

References

Details

(Keywords: crash)

Attachments

(3 files, 3 obsolete files)

Attached file 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.
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.
Attached patch last-update.logSplinter Review
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.
Duplicate of this bug: 523963
Severity: normal → critical
Keywords: crash
Attached patch patch (obsolete) — Splinter Review
Simple fix... surprised this wasn't found a long time ago.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #412123 - Flags: review?(ted.mielczarek)
Comment on attachment 412123 [details] [diff] [review]
patch

Actually, this should cleanup after itself as well
Attachment #412123 - Flags: review?(ted.mielczarek)
Attached patch patch rev2 (obsolete) — Splinter Review
Attachment #412123 - Attachment is obsolete: true
Attachment #412125 - Flags: review?(ted.mielczarek)
(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
Attachment #412125 - Attachment is obsolete: true
Attachment #412127 - Flags: review?(ted.mielczarek)
Attachment #412125 - Flags: review?(ted.mielczarek)
(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().
(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.
(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 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);
Attachment #412127 - Attachment is obsolete: true
Attachment #412127 - Flags: review?(ted.mielczarek)
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
Attached patch patch rev4Splinter Review
all better. The reason for the additional changes is the updated style guide.
Attachment #412177 - Flags: review?(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?
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.
Attachment #412177 - Flags: review?(ted.mielczarek) → review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/b89fd003bdd9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 412177 [details] [diff] [review]
patch rev4

Drivers, this fixes a crash in the updater that breaks updating badly.
Attachment #412177 - Flags: approval1.9.2?
Comment on attachment 412177 [details] [diff] [review]
patch rev4

a192=beltzner
Attachment #412177 - Flags: approval1.9.2? → approval1.9.2+
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.
btw: I verified this with today's nightly partial
You need to log in before you can comment on or make changes to this bug.