410900 - Applications pref pane is empty

Status: VERIFIED FIXED

(Firefox :: File Handling, defect)

Description

[Reed Loden [:reed]]

Some check-in today caused the applications pref pane to become completely empty.
Probably bug 408957, which also caused bug 410894.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008010504 Minefield/3.0b3pre

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Comment 2

[Brendan Eich [:brendan]]

Even if JS2/ES4 makes the switch body-block scope let declarations, that's not what this code (or the downloads.js code) wants -- this code wants implicit blocks around switch-case statement lists. But due to fall-through without break, these cannot be scoped without analyzing control flow.

More sane but still questionable would be to scope all let decls in all case code to the switch's body block, which is what JS1.7 did.

But there's always explicit block containing let decls, or let blocks if putting the decls in the head (with the ability to use outer synonyms in initializers) is important. It's up to myk, but my 2 cents favor keeping let but using explicit let blocks.

/be

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: use explicit/let blocks

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 295570 [details] [diff] [review]
use explicit/let blocks

I agree with Brendan; this is the right fix. r=myk

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Simple alive test

Comment 7

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/browser/components/preferences/applications.js 1.31

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 295748 [details] [diff] [review]
Simple alive test

Thanks for adding a test for this!

>diff --git a/browser/components/preferences/Makefile.in b/browser/components/preferences/Makefile.in

>+ifdef MOZ_MOCHITEST
>+DIRS += tests
>+endif

Nit: it seems like other browser chrome mochitests have been using "test" singular as their directory name, so perhaps this should do the same.  On the other hand, lots of other tests are using the plural version, so it probably doesn't matter.


>diff --git a/browser/components/preferences/tests/Makefile.in b/browser/components/preferences/tests/Makefile.in

>+libs::	$(_BROWSER_FILES)
>+	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/$(relativesrcdir)

Despite the apparent duplication, this should be:

+	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)

Which puts the tests in:

_tests/testing/mochitest/browser/browser/components/preferences

So that you can run them from the command line via:

perl runtests.pl --browser-chrome --test-path=browser/components/preferences

r=myk with this change


>diff --git a/browser/components/preferences/tests/browser_bug410900.js b/browser/components/preferences/tests/browser_bug410900.js

>+  var doc = win && win.document;

Why isn't this just "var doc = win.document"?

Comment 9

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #8)
> >+  var doc = win && win.document;
> 
> Why isn't this just "var doc = win.document"?
> 

Just a leftover from a previous iteration - I removed this and corrected the nsinstall path on checkin.

Comment 10

[Reed Loden [:reed]]

I disabled the last part of this test due to a failure that appeared after bug 397815 landed.

chrome://mochikit/content/browser/browser/components/preferences/tests/browser_bug410900.js
	PASS - Pref window opened
	PASS - Specified pane was opened
	PASS - handlersView is present
	FAIL - App handler list populated - chrome://mochikit/content/browser/browser/components/preferences/tests/browser_bug410900.js

I downloaded an hourly to check this, and the applications pane looked fine, so I'm guessing it's a problem with the test itself?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> I disabled the last part of this test due to a failure that appeared after bug
> 397815 landed.

I don't see any way that that patch could have affected this test. It might have just been a fluke.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(A fluke that that test failed at the same time that that patch was checked in, I mean. The test itself could have failed for a number of reasons.)

Comment 13

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3 ID:2008020514

- > Verified fixed

Comment 14

[Elias Hasle]

I have this problem in Firefox 3 Beta 5 on Xubuntu, with settings imported from my previous installation of Firefox 2 on LinuxMint 4. It is annoying.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

The test failed again
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/preferences/tests/browser_bug410900.js | handlersView is present
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/preferences/tests/browser_bug410900.js | App handler list populated

Using timeout to see if window is opened and the document for it is loaded is
error prone. Would be better if load event could be used.

Comment 16

[Elias Hasle]

I still have the same problem, in FireFox 3.0 (rv.1.9. What's that?) on Xubuntu.

Comment 17

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #16)
> I still have the same problem, in FireFox 3.0 (rv.1.9. What's that?) on
> Xubuntu.
> 

This bug has to do with with a short-lived change to JavaScript engine that caused this pane to fail to load due to syntax errors. If you're having issues with it in any build beyond beta 3 and are using a build from a package manager, you should file a bug in your distribution's bug tracker. Otherwise file a new one here.