635546 - [XP] No restart prompt after installing/updating non-restartless extensions and Java installed on the system

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Broken in Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110219 Firefox/4.0b12pre
Works with Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110218 Firefox/4.0b12pre

Installing an add-on (independent if web install or by file drag and drop) shows a doorhanger that the add-on has been installed and can be used, the button shows 'Open add-ons manager', even for normal non-bootstrapped, non-jetpack add-ons. The add-ons manager shows them as installed, but of course they are not active.

Comment 1

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

I can't reproduce that on Mac. I always get the restart notification for non-restartless add-ons. Given the above regression range we have:

PASS: http://hg.mozilla.org/mozilla-central/rev/1eed87fac9ce
FAIL: http://hg.mozilla.org/mozilla-central/rev/657c2a92ee2b

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1eed87fac9ce&tochange=657c2a92ee2b

I cannot find anything related in this list. Sebastian, can you reproduce that also in a fresh profile?

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Yes, I reproduced in a fresh profile, even with trunk downloaded again. Fund now something in the error console (installing the WikiTrust extension for the first time will not show the install confirmation prompt, keeps showing the full download bar; works on second try). https://addons.mozilla.org/en-US/firefox/search/?q=wikitrust&cat=all&x=0&y=0

Error: ERROR addons.manager: Exception calling provider startup: [Exception... "Component returned failure code: 0x80520011 (NS_ERROR_FILE_NAME_TOO_LONG) [nsIFile.directoryEntries]"  nsresult: "0x80520011 (NS_ERROR_FILE_NAME_TOO_LONG)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: recursiveLastModifiedTime :: line 1125"  data: no]
Source File: resource://gre/modules/XPIProvider.jsm
Line: 1125

Error: ERROR addons.xpi: Error creating statement getVisibleAddonForID (SELECT internal_id, id, location, version, type, internalName, updateURL, updateKey, optionsURL, aboutURL, iconURL, icon64URL, defaultLocale, visible, active, userDisabled, appDisabled, pendingUninstall, descriptor, installDate, updateDate, applyBackgroundUpdates, bootstrap, skinnable, size, sourceURI, releaseNotesURI FROM addon WHERE visible=1 AND id=:id)
Source File: resource://gre/modules/XPIProvider.jsm
Line: 3997

Error: ERROR addons.manager: Exception calling provider getAddonByID: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIINIParserFactory.createINIParser]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: XPIDB_getActiveBundles :: line 3804"  data: no]
Source File: resource://gre/modules/XPIProvider.jsm
Line: 3804

In 20110218, the Java Quick Starter 1.0 and Microsoft .NET Framework Assistant 0.0.0 are installed by default in the new profile, in 20110219 they are missing.

Comment 4

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

Given the error you are seeing we fail in nsIFile.directoryEntries with NS_ERROR_FILE_NAME_TOO_LONG. Where is your profile located? Does it exceed the 255 character limit for one of your installed add-ons?

Comment 5

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

So the .net framework registers itself via the Windows Registry. Ehsan, I wonder if that has been caused by your patch on bug 634819.

Comment 6

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

Sebastian, can you please temporarily remove the registry key for the .net framework and try again if that fixes the issue?

Comment 7

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

Also for anyone who can see this issue, do you have a localized version of Windows installed?

Comment 8

[JustOff]

Error: ERROR addons.manager: Exception calling provider startup: [Exception... "Component returned failure code: 0x80520011 (NS_ERROR_FILE_NAME_TOO_LONG) [nsIFile.directoryEntries]"  nsresult: "0x80520011 (NS_ERROR_FILE_NAME_TOO_LONG)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: recursiveLastModifiedTime :: line 1125"  data: no]
Source File: resource://gre/modules/XPIProvider.jsm
Line: 1125

as for me, this error no longer occurs after deleting sun java registry key ([e-mail] REG_EXPAND_SZ C:\Program Files\Java\jre6\lib\deploy\jqs\ff) from HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\Firefox\Extensions and not depends of .net key

Comment 9

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

Thanks! That would make it really a regression from bug 634819. Requesting blocking as it is a fairly intensive regression.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Confirming that removing the registry key for java fixes the problem.

Comment 12

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

Ok, I can reproduce it now with Java installed. This is Windows XP only issue and doesn't happen on Windows Vista or 7. Seems like it's related to the older registry format which is now broken due to bug 634819.

Steps:
1. Install latest Java version on your system
2. Create a fresh profile
3. Install any non-restartless extension (i.e. adblock plus)
4. Click 'options' if available to cause a freeze of Firefox

Dave, I think step 4 is a different beast we should try to fix for the AOM. We shouldn't give the user the option to open the preferences of non-restartless add-ons as long those haven't been correctly installed.

Comment 13

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

I think this should be a beta 12 blocker. CC'ing Christian.

Comment 15

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

Similar behavior appears for updates, which doesn't let us delete the old extension files under the extensions/trash folder.

Comment 16

[Dimitri Saltanov]

Also notice the absense of the Default theme as well as other 'built-in' addons (Roboform in may case), and inability of the XPI provider to collect any manual additions/deletions to the extensions folder. It seems to just fail to read extension files anymore after the NS_ERROR_FILE_NAME_TOO_LONG on startup, and in particular chrome.manifest files which effectively leads it to think that everything installed is restartless.

Also confirming that the presence of the Java reg key is the trigger for this. My XP in non-localized.

Comment 19

[Johnathan Nightingale [:johnath]]

I agree that failing to prompt to restart after installing an addon is a blocker, but I don't think this needs to block a beta. Disagree, Dave?

Comment 20

[JustOff]

Please note, that this regression not only about failing to prompt to restart after installing an addon, but addons manager is completely broken when java installed and none of addons and themes can be installed or managed even with browser restart if [e-mail] key is in registry!

Comment 21

[(no longer active)]

I think I've found the cause of the bug.  ExpandEnvironmentStringsW (used in nsWindowsRegKey::ReadStringValue) returns the length of the buffer including the terminating null character, and we fail to account for that character, so the length of the string containing "C:\Program Files\Java\jre6\lib\deploy\jqs\ff" would be returned as 45 (instead of 44).

Later on, when we try to append to this path at <http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#1126>, we end up appending the string after the terminating null character, which means that when the string is passed to the Windows APIs, the appended part would be hidden, which makes the recursion in recursiveLastModifiedTime infinite.  Later on, we bail out here <http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#518> when the length of the path ends up exceeding MAX_PATH.

Comment 22

[(no longer active)]

Attachment: Patch (v1)

Comment 23

[(no longer active)]

Attachment: Patch (v1)

With the correct patch header...

Comment 24

[(no longer active)]

I'd recommend that this should block beta12, as it cripples the add-on manager for all XP users who have Java installed on their system, and we wouldn't be able to get useful feedback with regard to the add-on manager from them if we release beta12 with this bug.

Comment 25

[Mike Shaver (:shaver emeritus)]

Yep, let's hustle.

Comment 26

[(no longer active)]

Comment on attachment 514097 [details] [diff] [review]
Patch (v1)

Benjamin, if you can get to this sooner, I can land it.

Comment 27

[(no longer active)]

Try server builds available at: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8cfa129c58bd/tryserver-win32/>

Comment 28

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

This tryserver build works fine. Thanks Ehsan. Also why does it only happen for the registry key of Java and not for the .net framework extension?

Comment 29

[(no longer active)]

(In reply to comment #28)
> This tryserver build works fine. Thanks Ehsan. Also why does it only happen for
> the registry key of Java and not for the .net framework extension?

The type of the Java registry value is REG_EXPAND_SZ, which triggers the bug.  The .NET registry value is REG_SZ, so it doesn't trigger this bug.

Comment 30

[Benjamin Smedberg]

Comment on attachment 514097 [details] [diff] [review]
Patch (v1)

Can we unit-test this?

Comment 31

[(no longer active)]

(In reply to comment #30)
> Comment on attachment 514097 [details] [diff] [review]
> Patch (v1)
> 
> Can we unit-test this?

Sure, will write a test for it.

Comment 32

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d14d4de010c5

Comment 33

[Jim Mathies [:jimm]]

(In reply to comment #31)
> (In reply to comment #30)
> > Comment on attachment 514097 [details] [diff] [review]
> > Patch (v1)
> > 
> > Can we unit-test this?
> 
> Sure, will write a test for it.

Based on a MXR search this code isn't tested at all. We should work up a suite of tests for this at some point. We could use xpc shell test so that we could read & write values using registry apis and confirm the interface is working. Care to file a follow up ehsan and cc me on it?

Comment 34

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

Verified fixed with Mozilla/5.0 (Windows NT 5.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12. Installation, upgrading, and removing of add-ons work as expected with Java installed.

Comment 35

[(no longer active)]

(In reply to comment #33)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > Comment on attachment 514097 [details] [diff] [review]
> > > Patch (v1)
> > > 
> > > Can we unit-test this?
> > 
> > Sure, will write a test for it.
> 
> Based on a MXR search this code isn't tested at all. We should work up a suite
> of tests for this at some point. We could use xpc shell test so that we could
> read & write values using registry apis and confirm the interface is working.
> Care to file a follow up ehsan and cc me on it?

Filed bug 636276.