506696 - [WinCE] updater.exe doesn't use the working directory when applying the update

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

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

There are also a few return codes that aren't correct for the functions they emulate in updater_wince.cpp

Comment 1

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

Attachment: Updated tests

Comment 2

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

Attachment: Updated tests rev2

Comment 3

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

Attachment: patch in progress

Comment 4

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

Comment on attachment 391306 [details] [diff] [review]
patch in progress

Benjamin, the updater on WinCE / WinMo needs to use the working-dir command line arg since they don't have a concept of a working directory. Since the arg is a WCHAR I went ahead and converted XP_WIN to use WCHAR where applicable. I've tested this on WinMo / Vista (I'll verify it works on WinCE tomorrow) and I sent the patch to the try server. There is some cleanup of updater_wince.cpp that I haven't done yet.

Comment 5

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

Comment on attachment 391306 [details] [diff] [review]
patch in progress

note to self: don't request review when half asleep

Comment 6

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

Attachment: patch in progress rev2 (sent to try server)

Comment 7

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

Attachment: patch rev1

Benjamin, the updater on WinCE / WinMo needs to use the working-dir command line arg since they don't have a concept of a working directory. Since the arg is a WCHAR I went ahead and converted XP_WIN to use WCHAR where applicable. I've tested this on WinMo / WinCE / WinVista and I sent the patch to the try server.

Comment 8

[Benjamin Smedberg]

Comment on attachment 391423 [details] [diff] [review]
patch rev1

At a rather skimming pace this looks correct.

Comment 9

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

Comment on attachment 391423 [details] [diff] [review]
patch rev1

Brad, could I get you to review these changes? Thanks

Comment 10

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

note: this passed the Linux try unit tests

Comment 11

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

Also passed the Mac OS X try unit tests

Comment 12

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

Also passed the WINNT try unit tests

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 391423 [details] [diff] [review]
patch rev1


>-# define mkdir(path, perms) _mkdir(path)
>+# define mkdir(path, perms) _wmkdir(path)

perhaps this should be tmkdir since it will be wide on some platforms and narrow on others

>   // Run from the specified working directory (see bug 312360).
>   if(NS_tchdir(workingDir) != 0)
>     LOG(("Warning: chdir failed"));
> 
>+  // Run from the specified working directory (see bug 312360).
>+  NS_tchdir(workingDir);
>+

why are you calling NS_tchdir(workingDir) twice?  Also, we don't need the comment twice

Comment 14

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

Thanks Brad!

(In reply to comment #13)
> (From update of attachment 391423 [details] [diff] [review])
> 
> >-# define mkdir(path, perms) _mkdir(path)
> >+# define mkdir(path, perms) _wmkdir(path)
> 
> perhaps this should be tmkdir since it will be wide on some platforms and
> narrow on others
Agreed and will change

> >   // Run from the specified working directory (see bug 312360).
> >   if(NS_tchdir(workingDir) != 0)
> >     LOG(("Warning: chdir failed"));
> > 
> >+  // Run from the specified working directory (see bug 312360).
> >+  NS_tchdir(workingDir);
> >+
> 
> why are you calling NS_tchdir(workingDir) twice?  Also, we don't need the
> comment twice
I had a bad merge and will fix this

Comment 15

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

Attachment: patch with comments addressed

Carrying forward r+

Comment 16

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

Attachment: patch

The previous patch was with -w and this cleans up a couple of bracket inconsistencies that have crept in over time. I'll check this in as soon as the tree is green.

Comment 17

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

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

along with updated tests
http://hg.mozilla.org/mozilla-central/rev/04a7efa84f87

Comment 18

[[e-mail]]

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

>+# define stat _stat
Out of interest, what is this supposed to do? (VC7.1 doesn't like it.)

Comment 19

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

On Windows _stat and _wstat takes the struct _stat as a param and iirc some of the WinCE work in one of the other file (archivereader.cpp I believe) required this for non WinCE.
http://msdn.microsoft.com/en-us/library/14h5k7ff%28VS.80%29.aspx

I'll take a look at reworking this in the next day or two.

Comment 20

[[e-mail]]

I was able to work around the problem by moving the #define stat _stat after the #include <sys/stat.h> and I also had to #define fstat _fstat too.
@@
-#ifndef WINCE
-# define stat _stat
-#endif
@@
 #ifdef WINCE
 #include updater_wince.h
+#elif defined(XP_WIN)
+#define stat _stat
+#define fstat _fstat
 #endif