Closed Bug 473417 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Application Update, defect, P1, major)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: pablo, Assigned: robert.strong.bugs)

References

()

Details

(Keywords: regression, verified1.9.1)

Attachments

(11 files, 8 obsolete files)

12.62 KB, image/jpeg
Details
870 bytes, text/plain
Details
862 bytes, text/plain
Details
173.00 KB, application/octet-stream
Details
2.05 KB, patch
Details | Diff | Splinter Review
2.46 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
172.00 KB, application/octet-stream
Details
4.78 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
480 bytes, text/plain
Details
3.04 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
12.19 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
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
Attached image screenshot
Yeah. I can confirm this aswell. 
starting the browserbefore killing the updater-process makes sure you don't kill it too early.
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?
Version: unspecified → Trunk
Attached file updater.ini
I'm using XP in Spanish.
Where should I get the locale information you need?
It happens every time
That should be enough... it gives me something to go on to figure out how to fix this.
This should block 1.9.1.

If no solution is found we should back out bug 399153
Flags: blocking1.9.1?
So this is seen with a Unicode updater.exe and an ANSI updater.ini file?
Attachment #356776 - Attachment mime type: application/octet-stream → text/plain
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
Attached file 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.
Thanks, we don't need any more updater.ini files in case someone else thinks they should attach one
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
(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.
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!
(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
(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.
Attached file test executable (obsolete) —
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
Attached patch possible patch (obsolete) — Splinter Review
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.
Assignee: nobody → robert.bugzilla
Attachment #356916 - Flags: review?(benjamin)
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.
Attachment #356917 - Attachment mime type: application/x-msdownload → application/octet-stream
My patch.  It is more brain-dead than Robert's patch, but also simpler and like I said it seems to fix the problem.
Attachment #356919 - Flags: review?(benjamin)
(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...
(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.
(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...
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.
(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... :/
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.
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.
(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?
(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.
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.
Attachment #356889 - Attachment is obsolete: true
(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
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.
(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.
Assignee: robert.bugzilla → ehsan.akhgari
Comment on attachment 356919 [details] [diff] [review]
Another possible patch

Make sure you remove the updater.cpp hunk before checkin ;-)
Attachment #356919 - Flags: review?(benjamin) → review+
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!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Ehsan, should it work today, or tomorrow after updater is updated?
(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
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
(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
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.
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #356916 - Flags: review?(benjamin)
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?
(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?
(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
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 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.
Attachment #356916 - Flags: review?(benjamin) → review-
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?
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?
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.
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.
Attached patch New patchSplinter Review
Attachment #356916 - Attachment is obsolete: true
Attachment #357772 - Flags: review?(benjamin)
Attachment #357772 - Flags: review?(benjamin) → review+
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.
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!
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
(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.
(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?
Attached file 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
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: ehsan.akhgari → robert.bugzilla
Attachment #358007 - Flags: review?(benjamin)
Attachment #358007 - Flags: review?(benjamin) → review+
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
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
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.
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
Whiteboard: [waiting on results of 269538 backout to see if it fixes]
Whiteboard: [waiting on results of 269538 backout to see if it fixes] → [waiting on results of 269538 backout to land]
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
Attachment #358007 - Attachment description: patch - use wchar for path and _wfopen on Windows → patch - use wchar for path and _wfopen on Windows (pushed to mozilla-central)
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [waiting on results of 269538 backout to land]
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
Whiteboard: [waiting on verification of fix before landing on mozilla-1.9.1]
Whiteboard: [waiting on verification of fix before landing on mozilla-1.9.1] → [waiting on verification of fix before landing on mozilla-1.9.1 comment #67]
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
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!!!
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1
Whiteboard: [waiting on verification of fix before landing on mozilla-1.9.1 comment #67]
btw: I have a c++ test for this in the works
Flags: in-testsuite?
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
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).
Definitely and thanks for the suggestions... I am going to add testing with multiple ini files as time permits.
Attached patch tests rev1 (obsolete) — Splinter Review
Attachment #359093 - Attachment is obsolete: true
Attachment #359375 - Flags: review?(ted.mielczarek)
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.
Marcia, this is for the updater binary... not the update wizard. See screenshot in attachment #356772 [details]
btw: Marcia's bug is bug 472728 and affects several parts of the app.
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.
Attachment #359375 - Flags: review?(ted.mielczarek) → review+
Attached patch patch rev2 (obsolete) — Splinter Review
Addresses review comments... carrying forward r+
Attachment #359375 - Attachment is obsolete: true
Attached patch tests patch rev3 (obsolete) — Splinter Review
bah... some cruft crept in.
Attachment #360574 - Attachment is obsolete: true
Attachment #360577 - Flags: review+
Attachment #360577 - Attachment description: patch rev3 → tests patch rev3
Attachment #360577 - Flags: review+ → review?(ted.mielczarek)
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?
Attached patch Tests for checkin (obsolete) — Splinter Review
I talked with Ted and he is fine with not adding the manifest
Attachment #360577 - Attachment is obsolete: true
Attachment #360772 - Flags: review+
Attachment #360577 - Flags: review?(ted.mielczarek)
Attached patch Tests as checked in (obsolete) — Splinter Review
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.
Attachment #360772 - Attachment is obsolete: true
Attachment #361088 - Flags: review+
I'll wait a cycle or several before pushing the tests to mozilla-1.9.1
Flags: in-testsuite? → in-testsuite+
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.
Attachment #361088 - Attachment is obsolete: true
Attachment #361103 - Flags: review+
Depends on: 477508
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
I'll take care of that in bug 477577
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Depends on: 479524
You need to log in before you can comment on or make changes to this bug.