473417 - updater.exe window is blank, and doesn't close

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Pablo Greco]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090113 Minefield/3.2a1pre Firefox/3.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090113 Minefield/3.2a1pre Firefox/3.0.4 ID:20090113035246

After the checkin of bug 399153, updater.exe window comes up on the top left corner (instead of centered), blank, and stays open after update is done. If you wait enough time, and kill updater.exe, minefield (same thing for shiretoko and shredder) start ok (manually), and you can confirm that update has been applied correctly.
Screenshot coming up

Reproducible: Always

Comment 1

[Pablo Greco]

Attachment: screenshot

Comment 2

[Andreas Treppe]

Yeah. I can confirm this aswell. 
starting the browserbefore killing the updater-process makes sure you don't kill it too early.

Comment 3

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

Please attach your updater.ini.

Just to verify... you are running Windows XP? Also, please provide OS locale information (is it English US or something else, ANSI code page modification)? Does it always do this?

Comment 4

[Pablo Greco]

Attachment: updater.ini

I'm using XP in Spanish.
Where should I get the locale information you need?
It happens every time

Comment 5

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

That should be enough... it gives me something to go on to figure out how to fix this.

Comment 6

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

This should block 1.9.1.

If no solution is found we should back out bug 399153

Comment 7

[(no longer active)]

So this is seen with a Unicode updater.exe and an ANSI updater.ini file?

Comment 8

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

The updater.ini is UTF-8. The updater.exe has not changed to Unicode... the string it reads are now UTF-8 instead of ANSI. Chances are it is the MultiByteToWideChar call

Comment 9

[Andreas Treppe]

Attachment: Shiretoko updater.ini

Here's my updater.ini
And i run Win XP as well.
The os has another default locale. (sv-SE) but that has never been an issue.

Comment 10

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

Thanks, we don't need any more updater.ini files in case someone else thinks they should attach one

Comment 11

[(no longer active)]

Andreas: have you been testing with 1.9.2a1pre or 1.9.1b3pre?  I assume the latter, because the file you attached contains Info, and not InfoText.

Pablo: can you reproduce this with 1.9.1b3pre as well?

Comment 12

[Andreas Treppe]

(In reply to comment #11)
> Andreas: have you been testing with 1.9.2a1pre or 1.9.1b3pre?  I assume the
> latter, because the file you attached contains Info, and not InfoText.
> 
> Pablo: can you reproduce this with 1.9.1b3pre as well?

Current version info-string:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090113 Shiretoko/3.1b3pre  

so yea. 1.9.1b3pre it is.

Comment 14

[(no longer active)]

So, the failure occurs at <http://hg.mozilla.org/mozilla-central/file/7165ce2d87c7/toolkit/mozapps/update/src/updater/readstrings.cpp#l86>.  I was reproducing this consistently, and suddenly it went fine!  IINM, the last thing I did before it was corrected was building objdir/browser/locales to regenerate updater.ini...


Robert, can you take a look into this while I head off to bed?  Thanks!

Comment 15

[Pablo Greco]

(In reply to comment #11)
> Pablo: can you reproduce this with 1.9.1b3pre as well?
yes, I can reproduce it on all three Shiretoko, Minefield and Shredder

Comment 16

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

(In reply to comment #14)
> So, the failure occurs at
> <http://hg.mozilla.org/mozilla-central/file/7165ce2d87c7/toolkit/mozapps/update/src/updater/readstrings.cpp#l86>.
>  I was reproducing this consistently, and suddenly it went fine!  IINM, the
> last thing I did before it was corrected was building objdir/browser/locales to
> regenerate updater.ini...
> 
> 
> Robert, can you take a look into this while I head off to bed?  Thanks!
Been looking at it and the one thing that comes to mind is that it may be reading the UTF-8 BOM as the first line under some circumstances. I'll see if I can come up with a quick workaround for it.

Comment 17

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

Attachment: test executable

If someone that can reproduce this bug could run this executable in a directory with their updater.ini I would appreciate it since I haven't been able to reproduce it. You should see two message boxes... the first one should have the text 'Software Update' and the second should have 'Minefield is installing your updates and will start in a few moments…' where Minefield is specific to the updater.ini from the app you got it from. If it fails it will display text to hopefully help further diagnose this bug. Thanks

Comment 18

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

Attachment: possible patch

Hey Benjamin, what I *think* is happening here is the UTF-8 BOM is being read in as a line which throws off the hack where it expects the ini name value pairs to be on specific lines. I started writing an ini parser and then realized we had one in the tree so I mostly copied this over from there. I haven't been able to reproduce this by installing the Spanish language on my system along with setting the ANSI locale to Spanish so I'm not positive this will fix it but I think it would be a good thing to do anyways.

If someone that is experiencing this would please try out the test executable I attached and report back I would know whether or not this will fix this bug.

Comment 19

[(no longer active)]

Attachment: Another test executable

I have another patch in the works.  To see if things fix this, please run copy this in your Firefox directory and double-click on it.  The updater window should appear at the center of screen with the text inside it (this executable only shows the dialog and exits).

I have been able to reproduce this intermittently, but it seems like this patch fixes this for me, as I have never been able to reproduce it with this patch applied.

I'll attach my patch shortly.

Comment 20

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

Attachment: additional patch so it doesn't fail if strings aren't available

Hey Ehsan, looking forward to seeing the patch

Comment 21

[(no longer active)]

Attachment: Another possible patch

My patch.  It is more brain-dead than Robert's patch, but also simpler and like I said it seems to fix the problem.

Comment 22

[(no longer active)]

(In reply to comment #20)
> Created an attachment (id=356918) [details]
> additional patch so it doesn't fail if strings aren't available
> 
> Hey Ehsan, looking forward to seeing the patch

I think this is a good thing to do anyway, but would it be appropriate to have some default (hard-coded and non-localized) text here as a fallback?  Without that, the dialog would still turn up empty if something goes wrong, and nothing would be more frustrating than an empty dialog...

Comment 23

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

(In reply to comment #21)
> Created an attachment (id=356919) [details]
> Another possible patch
> 
> My patch.  It is more brain-dead than Robert's patch, but also simpler and like
> I said it seems to fix the problem.
That is pretty close to the proof of concept I wrote.

Do you think the UTF-8 BOM is what causing this? It is the only thing that makes sense to me.

(In reply to comment #22)
> (In reply to comment #20)
> > Created an attachment (id=356918) [details] [details]
> > additional patch so it doesn't fail if strings aren't available
> > 
> > Hey Ehsan, looking forward to seeing the patch
> 
> I think this is a good thing to do anyway, but would it be appropriate to have
> some default (hard-coded and non-localized) text here as a fallback?  Without
> that, the dialog would still turn up empty if something goes wrong, and nothing
> would be more frustrating than an empty dialog...
I thought about that and figured it could be decided after the initial fix is decided upon.

Comment 24

[(no longer active)]

(In reply to comment #23)
> Do you think the UTF-8 BOM is what causing this? It is the only thing that
> makes sense to me.

Yes, possibly.  All in all, fgets is not ready for reading UTF-8 files it seems.

BTW, to state the obvious, the updater.cpp changes should be ignored, as they were added only for being able to run the executable and see what the dialog would look like...

/me wishes that updater could be written in XUL...

Comment 25

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

If you can still fail with the original code then you could take a look at the string that is failing to determine if it is reading the wrong line from the ini file.

Comment 26

[(no longer active)]

(In reply to comment #25)
> If you can still fail with the original code then you could take a look at the
> string that is failing to determine if it is reading the wrong line from the
> ini file.

The problem is I was never able to reproduce this consistently, and I couldn't  determine what exactly causes this to happen... :/

Comment 27

[(no longer active)]

But anyway I am 100% sure that the failure happened on <http://hg.mozilla.org/mozilla-central/file/7165ce2d87c7/toolkit/mozapps/update/src/updater/readstrings.cpp#l86>, and the only possible reason would be that the code is reading one extra line before the for loop.  Hence the patch should be able to fix it, since it doesn't rely on there being exactly two "junk" lines in updater.ini before the key=value pairs.

Comment 28

[Pablo Greco]

I've run the second test executable, and the dialog appears correctly. The only problem is that it stays open, I had to kill the process with task manager.

Comment 29

[(no longer active)]

(In reply to comment #28)
> I've run the second test executable, and the dialog appears correctly. The only
> problem is that it stays open, I had to kill the process with task manager.

Thanks for testing this.  The dialog staying open is expected; I just wanted to make sure that the dialog shows up correctly.  Did you test Robert's first test executable as well?

Comment 30

[Pablo Greco]

(In reply to comment #29)
> (In reply to comment #28)
> > I've run the second test executable, and the dialog appears correctly. The only
> > problem is that it stays open, I had to kill the process with task manager.
> 
> Thanks for testing this.  The dialog staying open is expected; I just wanted to
> make sure that the dialog shows up correctly.  Did you test Robert's first test
> executable as well?
I just did it, and it shows nothing.

Comment 31

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

Attachment: test executable using the patch

I suspect the executable was run from a dir without the updater.ini in it.

Could someone please try this exe and make sure the updater.ini is in the same dir. This just init's the dialog and then exits after 5 seconds.

Comment 32

[Pablo Greco]

(In reply to comment #31)
> Created an attachment (id=356958) [details]
> test executable using the patch
> 
> I suspect the executable was run from a dir without the updater.ini in it.
> 
> Could someone please try this exe and make sure the updater.ini is in the same
> dir. This just init's the dialog and then exits after 5 seconds.

I've just ran it on a dir with only updater.exe and updater.ini from today's minefield. Dialog appears centered, with all texts, and disappears after a few seconds. All ok

Comment 33

[Benjamin Smedberg]

I'm having trouble following this bug... am I supposed to pick between the two approaches here? :rs, which one do you prefer?

If we're going for the more full-blown INI parser, I'm a little unhappy about the comment/requirement:

+    // The first entry in the [Strings] section of the updater.ini must be
+    // Title and the second entry must be either Info or InfoText

Is there a reason we can't just keep parsing?

Also, you could clean the sections bit up more with a `bool isStringsSection` rather than having to strcmp every line.

Comment 34

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

(In reply to comment #33)
> I'm having trouble following this bug... am I supposed to pick between the two
> approaches here? :rs, which one do you prefer?
Ehsan's approach is essentially the first method I went with so I am for the most part fine with either method.

The main concerns I have with the...

patch using fgets is that using fgets apparently acts differently on different Windows locales and that the patch only strips \n where the ini may contain \r\n though that doesn't appear have an adverse affect.

patch with the INI parser besides your comments is that it changes more though that is mitigated by it being from nsINIParser.cpp.

Having said that let's go with Ehsan's patch and if we decide later that this needs a full INI parser then we'll add that then.

Comment 35

[Benjamin Smedberg]

Comment on attachment 356919 [details] [diff] [review]
Another possible patch

Make sure you remove the updater.cpp hunk before checkin ;-)

Comment 36

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/0c5fb2ad3414

Pablo, can you please verify this on trunk before I land it on 1.9.1?  Thanks!

Comment 37

[Pablo Greco]

Ehsan, should it work today, or tomorrow after updater is updated?

Comment 38

[Pablo Greco]

(In reply to comment #37)
> Ehsan, should it work today, or tomorrow after updater is updated?
Never mind, updating to 20090115033950 didn't work. Will report back tomorrow

Comment 39

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

Pablo, the patch applied to the following hourly build
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1232006634/

Could you download that and try updating? Thanks

Comment 40

[Pablo Greco]

(In reply to comment #39)
> Pablo, the patch applied to the following hourly build
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1232006634/
> 
> Could you download that and try updating? Thanks

Downloaded the hourly, but it didn't work. Guess I'll have to wait until the next nightly

Comment 41

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

Hmmm... I verified that build had the patch. Ehsan, if for some reason the patch you landed doesn't work I suggest opening the file to read as binary (see the patch I attached), etc.

Comment 42

[(no longer active)]

(In reply to comment #41)
> Hmmm... I verified that build had the patch. Ehsan, if for some reason the
> patch you landed doesn't work I suggest opening the file to read as binary (see
> the patch I attached), etc.

This is a mystery to me, but yes, reading the file as binary might be safer...  Re-opening.

Comment 43

[Frank Wein [:mcsmurf]]

Doesn't he need to update to a build with the patch and then with the next update after that it will work again (as the first update happens with the old update.exe) or did I miss something?

Comment 44

[(no longer active)]

(In reply to comment #43)
> Doesn't he need to update to a build with the patch and then with the next
> update after that it will work again (as the first update happens with the old
> update.exe) or did I miss something?

I was under the impression that Pablo did exactly this.  Pablo, can you please confirm?

Comment 45

[Pablo Greco]

(In reply to comment #44)
> (In reply to comment #43)
> > Doesn't he need to update to a build with the patch and then with the next
> > update after that it will work again (as the first update happens with the old
> > update.exe) or did I miss something?
> 
> I was under the impression that Pablo did exactly this.  Pablo, can you please
> confirm?
I downloaded the hourly Rob pointed, and did a Full update to 20090115033950. So I can confirm that's what I did

Comment 46

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

btw: I verified that build had the patch by adding a couple of lines to the beginning of the updater.ini and updating which completed successfully. I also verified the previous build didn't have the patch by adding a couple of lines to the beginning of the updater.ini and updating which did not complete successfully. It wouldn't hurt to verify by trying to update from yesterday's nightly to today's in case I pasted the wrong url though I am pretty sure I didn't.

Comment 47

[Benjamin Smedberg]

Comment on attachment 356916 [details] [diff] [review]
possible patch

>diff --git a/toolkit/mozapps/update/src/updater/readstrings.cpp b/toolkit/mozapps/update/src/updater/readstrings.cpp


>+const char*
>+NS_strspnp(const char *delims, const char *str)

>+char*
>+NS_strtok(const char *delims, char **str)

might as well make these static

>+    if (token[0] == '[') { // section header!
>+      ++token;
>+      currSection = token;

Since we really only care about the [Strings] section, use a `PRBool inStringsSection` instead of currSection. That also lets you get rid of

>+    if (!currSection || strcmp(currSection, "Strings") != 0) {

The strcmp here.

>+    // The first entry in the [Strings] section of the updater.ini must be
>+    // Title and the second entry must be either Info or InfoText
>+    if (strcmp(key, "Title") == 0) {
>+      strcpy(results->title, token);
>+    }
>+    else if (strcmp(key, "Info") == 0 || strcmp(key, "InfoText") == 0) {
>+      strcpy(results->info, token);
>+      return OK;
>+    }

I don't like this at all...

1) Need to verify that the string isn't longer than MAX_TEXT_LEN before doing a strcpy
2) requiring Info/InfoText to be last seems fragile. I'd prefer we just kept reading to the end of the file. You probably need some kind of flag to ensure you've read both values.

Comment 48

[Pablo Greco]

Rob, I originally tested updating form 20090114(nightly) to 20090115 and didn't work (updater wasn't updated yet)
Updating from hourly with updated updater.exe to 20090115 failed too.
And just to confirm that this is not working, updating from 20090115 to 20090116 failed too. Is there anything I can do in my machine to help?

Comment 49

[notagain]

I can confirm this happening to the latest nightlies of Minefield and Shredder under Greek WinXP.

Been having this issue for some days now and I just waited thinking it might go away after the next one build or two. Since it persisted, I googled around for it and I ended up here.

Still having it with 20090116035550, I downloaded the patched updater.exe and run it on the same dir with updater.ini. First time it opened centered on the screen, but no text in it. Tried a couple of times again and again after it closed (stayed open for a few secs) and it came up correct every single time both position and text.

So, I updated to 20090119074946 and I am still having the issue. What do I need to do to get rid of it? Are we waiting for a fix?

Comment 50

[(no longer active)]

Robert: do you want me to address Benjamin's review comments?

notagain: Which patched updater.exe did you download and try out?

And by the way, yes we are trying to come up with a fix.

Comment 51

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

Ehsan, if you can that would be great... I probably won't have time to look at this for a day or two. It might be simpler to go with the same approach you took with the patch you checked in and reading as binary.

Comment 52

[(no longer active)]

Attachment: New patch

Comment 53

[Benjamin Smedberg]

Comment on attachment 357772 [details] [diff] [review]
New patch

>diff --git a/toolkit/mozapps/update/src/updater/readstrings.cpp b/toolkit/mozapps/update/src/updater/readstrings.cpp

>+    // The first entry in the [Strings] section of the updater.ini must be
>+    // Title and the second entry must be either Info or InfoText

Please remove this comment since it is no longer correct.

Comment 54

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/2deba79aa0b5

Pablo: can you please try using the next hourly/nightly, and then updating to the next one using that build to see if this is fixed?  Thanks!

Comment 55

[Pablo Greco]

(In reply to comment #54)
> http://hg.mozilla.org/mozilla-central/rev/2deba79aa0b5
> 
> Pablo: can you please try using the next hourly/nightly, and then updating to
> the next one using that build to see if this is fixed?  Thanks!
I don't get it. I downloaded an hourly from http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1232525007/
which was build with changeset 68391d886801, so it should include the new updater.exe. But after nightly finally came out, it failed the same way as before. I don't know what else I can do to help you test this.

Comment 56

[(no longer active)]

(In reply to comment #55)
> I don't get it. I downloaded an hourly from
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1232525007/
> which was build with changeset 68391d886801, so it should include the new
> updater.exe. But after nightly finally came out, it failed the same way as
> before. I don't know what else I can do to help you test this.

Can you please provide a details set of steps which you followed during testing (quite detailed, like downloading the zip package, extracting it in C:\path, ...)?  Have you seen this issue on other computers?  Do you see this with both the installer and the ZIP package?  What about on a clean profile?

Comment 57

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

Attachment: INI Parser test

Pablo, could you take the text in this attachment, paste it into the Error Console's (Tools -> Error Console) Code textbox, and click Evaluate? This must be done in Minefield. This should display two messageboxes with the first one displaying the dialog title and the second one displaying the dialog text for the Software Update window. Thanks

Comment 58

[Pablo Greco]

Guys, forget about the parser. It has nothing to do with the problem.
If I'm not mistaken, updater.exe and updater.ini are copied to local Settings folder under the user's folder, right?
So I tested the original "test executable using patch" that robert posted. It shows the right strings from c:\test but fails from c:\documents and settings\pablo\Configuración Local\Temp\test. So I'm guessing you should be able to reproduce this just creating a user with any non ASCII char on user name.
Hope it helps.

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is still broken as Pablo pointed out. I'm able to reproduce the issue with some simple steps:

1. Get a minefield nightly build from yesterday
2. Unzip the file into a folder with non-ascii characters like: Mínefíeld
3. Start Minefield
4. Manual check for an update
5. Download update and restart Minefield

=> When the updater comes up the window is blank. So you only have to use a non-ascii character in a folder name to reproduce it. => Reopen

Comment 60

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

Attachment: patch - use wchar for path and _wfopen on Windows (pushed to mozilla-central)

I was able to reproduce using the steps as outlined by Pablo.

I would like to keep the code as checked in by Ehsan since he was seeing a problem with parsing the lines as well.

Comment 61

[Vlado Valastiak [:wladow]]

Robert, I don't how useful is this gonna be for you, but I can reproduce 'non-ascii characters in the path' issue on Win Vista too

Comment 62

[Fernando Piera]

Just for your information Seamonkey night update Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090121 SeaMonkey/2.0a3pre
works fine.  This bug is not present

Comment 63

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

This bug won't affect all installs and it is present on SeaMonkey if it uses software update as well as all toolkit based apps that use software update under certain conditions.

Comment 64

[Fernando Piera]

Just for your information Seamonkey night update Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090121 SeaMonkey/2.0a3pre
works fine.  This bug is not present

Comment 65

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

Comment on attachment 358007 [details] [diff] [review]
patch - use wchar for path and _wfopen on Windows (pushed to mozilla-central)

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

Comment 66

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

Going to wait on landing this and other patches on mozilla-1.9.1 after seeing if Pablo can still reproduce with this patch on a Minefield build.

Comment 67

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

Pablo, below are steps to test this before the next nightly after a build with the patch is available.

Download and install a build with the patch. I'll post a link to it after a build is available

In about:config add a new string with a name of
app.update.url.override
and a value of
https://aus2.mozilla.org/update/3/Firefox/3.2a1pre/20090120033555/WINNT_x86-msvc/en-US/nightly/Windows_NT%205.1/default/default/update.xml

Check for updates and then download and install the update. The partial will likely fail and you will be prompted to install the complete update. Download and install the complete update. This will install today's nightly build.
Post back in this bug whether or not it was successful.
In about:config reset the app.update.url.override preference

Comment 68

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

A build with the patch is now available
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1232582505/

Please follow the steps in comment #67

Comment 69

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

Using the build as noted in comment #68 and the steps as noted in comment #67 this is fixed for me. I would like verification first from Pablo or someone else that has been experiencing this bug before pushing the patches to mozilla-1.9.1. Thanks

Comment 70

[Pablo Greco]

It's fixed for me too Rob, all good now. The test in comment #67 worked just as you said.
I also made the same test as yesterday, but changing updater.exe from the hourly you pointed out in comment #67 and also worked.
I'm glad to say this is fixed for me now.
Thanks!!!

Comment 71

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

Thanks Pablo!

Pushed all three patches to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9636b642adf5
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2f71a04b0d9d
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/457a601e73ea

Also marking verified fixed on trunk since both the reporter and myself verified this.

Comment 72

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

btw: I have a c++ test for this in the works

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090124 Shiretoko/3.1b3pre (.NET CLR 3.5.30729) ID:20090124033656

Comment 74

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

Attachment: tests in progress

Comment 75

[(no longer active)]

This ini layout may also be worth testing:

Title=blah

[Strings]

Title=foo

Info=bar

InfoText=baz

[AnotherSection]

Info=foobar

This tests the correct skipping of irrelevant values, and correct overriding of Info with InfoText.  BTW, testing with additional ini files with either or both of Title and Info removed may also be helpful (the test should expect failure).

Comment 76

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

Definitely and thanks for the suggestions... I am going to add testing with multiple ini files as time permits.

Comment 77

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

Attachment: tests rev1

Comment 78

[Marcia Knous [:marcia]]

I am running today on Win Vista using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090128 Shiretoko/3.1b3pre. When I tried to update to the latest nightly I got the blank software update dialog with nothing showing but the Back, Finish and Cancel key. Pressing any of those keys seems to do nothing.

Is it possible this issue still can persist on Vista? I am running side by side installs of Minefield and 1.9.1 on this Vista machine.

Comment 79

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

Marcia, this is for the updater binary... not the update wizard. See screenshot in attachment #356772 [details]

Comment 80

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

btw: Marcia's bug is bug 472728 and affects several parts of the app.

Comment 81

[(not currently active) Ted Mielczarek]

Comment on attachment 359375 [details] [diff] [review]
tests rev1

 XPCSHELL_TESTS = \
-  unit \
-  $(NULL)
+	unit \
+	$(NULL)

Any particular reason you swapped two spaces for tabs? I prefer the two-space indent myself.

+bug473417dir = test_bug473417-�

I'm impressed that this actually works in make.

+	$(RM) -rf $(FINAL_TARGET)/$(bug473417dir)/ && mkdir $(FINAL_TARGET)/$(bug473417dir)/

We usually use $(NSINSTALL) -D instead of mkdir, but I'll understand if it chokes on the unicode. Also, $(FINAL_TARGET) is generally $(DIST)/bin, unless you override it somehow. I don't think we really want to stick test files in that directory. Can't you just put this in $(DEPTH)/_tests/updater or something?

+ * IMPORTANT!!!
+ * The binary produced for this test doesn't have a Windows manifest file

Tragically hilarious. You could stick a .manifest in the srcdir and get around this, no?

+	printf("Running TestAUSReadStrings tests\n");

Spurious tab?

I sort of just skimmed your actual test program, but it looks sane enough.

Comment 82

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

Attachment: patch rev2

Addresses review comments... carrying forward r+

Comment 83

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

Attachment: tests patch rev3

bah... some cruft crept in.

Comment 84

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

Comment on attachment 360577 [details] [diff] [review]
tests patch rev3

Hey Ted, in thinking on this we have a bunch of c++ tests that don't have a manifest included that would fail. I'm tempted to just leave this without the manifest. Also, to embed a manifest I have to use PROGRAM instead of SIMPLE_PROGRAMS. Thoughts?

Comment 85

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

Attachment: Tests for checkin

I talked with Ted and he is fine with not adding the manifest

Comment 86

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

Attachment: Tests as checked in

pushed to mozilla-central with a green tree... w00t!
http://hg.mozilla.org/mozilla-central/rev/f8009c23d516

I also changed the update Makefile.in so tests don't run unless MOZ_UPDATER is defined since several of the tests require the updater binary.

Comment 87

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

I'll wait a cycle or several before pushing the tests to mozilla-1.9.1

Comment 88

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

Attachment: tests as checked in w/ compiler warning fixes and only use a unicode char for the test dir on Windows

Pushed to mozilla-1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3ab95ecabc07

Mac Thunderbird trunk was failing due to not being able to create the dir with the Unicode char so the test only uses a dir with a Unicode char on Windows since that is what this bug is about.

Comment 89

[John Daggett (:jtd)]

Robert, this patch doesn't compile in all situations, your code is assuming users are building on Latin-based versions of Windows.  Under Japanese Windows, the compiler spits out a bunch of bad encoding errors and the compile fails.

Using UTF-8 strings directly in code like this trips up builds under Japanese Windows, since the compiler is assuming (argh!) that source files are in the default codepage of the system (i.e. codepage 932 (Shift-JIS) vs. 1252 (Windows Latin-1)).

There might be a way of forcing the compiler to interpret the file as a UTF-8 file by adding a BOM but that will probably break builds on other platforms with compilers that don't understand BOM's.  The safest way would be to simply escape the UTF-8 strings you're using:

"Title Test - 测试 測試 Δοκιμή テスト Испытание"

would become

"Title Test - \xE6\xB5\x8B\xE8\xAF\x95 \xE6\xB8\xAC\xE8\xA9\xA6 \xCE\x94\xCE\xBF\xCE\xBA\xCE\xB9\xCE\xBC\xCE\xAE \xE3\x83\x86\xE3\x82\xB9\xE3\x83\x88 \xD0\x98\xD1\x81\xD0\xBF\xD1\x8B\xD1\x82\xD0\xB0\xD0\xBD\xD0\xB8\xD0\xB5"

You could also read these strings in from a file, that would give you full control over the assumed encoding.

Comment 90

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

I'll take care of that in bug 477577