1246614 - Clean system add-ons logs error if feature directory is not in profile

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Frank-Rainer Grahl (:frg)]

After bug 1192921 the clean up routine logs errors if the feature directory does not exist in the profile. This happens in all comm-central applications but according to Dave Townsend also in Firefox. The Error should be downgraded to a warning.

>> Timestamp: 2/7/2016 12:30:22 PM
>> Error: 1454844622328	addons.xpi	ERROR	Failed to clean updated 
>> system add-ons directories.: Win error 2 during operation 
>> DirectoryIterator.prototype.next on file C:\Users\NO1\AppData\Roaming
>> \Mozilla\SeaMonkey\Profiles\es9uf9xh.default\features 
>> (The system cannot find the file specified.)
>> ((unknown module)) No traceback available
>> Source File: resource://gre/modules/Log.jsm
>> Line: 751

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1246614-clean_system_addons_error.patch

Downgrade the errors to warnings and change message of first occurence to distinguish it better from the first one.

Tested with suite VS2015 en-US x64 build

>> User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 >> Firefox/47.0 SeaMonkey/2.44a1
>> Build identifier: 20160208131631

>> 1454934377719	addons.xpi	WARN	Failed to clean updated system 
>> add-ons directories.: Win error 2 during operation 
>> DirectoryIterator.prototype.next on file C:\Users\NO1\AppData\Roaming\Mozilla
>> \SeaMonkey\Profiles\es9uf9xh.default\features (The system cannot find the 
>> file specified.) ((unknown module)) No traceback available

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 8716914 [details] [diff] [review]
1246614-clean_system_addons_error.patch

Review of attachment 8716914 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to keep these messages as errors in the case where something is actually broken. Instead we should just hide the case that is perfectly normal, where the base directory is missing. So for the first exception just check if e.becauseNoSuchFile is true and if so just return without logging anything.

Comment 3

[Frank-Rainer Grahl (:frg)]

Dave,

 not even logging an info message when the directory is not there?

Comment 4

[Dave Townsend [:mossop]]

(In reply to Frank-Rainer Grahl from comment #3)
> Dave,
> 
>  not even logging an info message when the directory is not there?

I don't think so, it's the normal state right now (and will continue to be for most applications) so there isn't anything to be gained from it.

Comment 5

[Frank-Rainer Grahl (:frg)]

Getting the iterator will not throw. I am thinking of putting an exists query in it at the start of the function but I have a hard time now reproducing it and do not want to submit another patch until I was able to test it. Clean routine didn't trigger during my tries. Stay tuned.

>> cleanDirectories: Task.async(function*() {
>>
>>   // System add-ons directory does not exist
>>  if (!(yield OS.File.exists(this._baseDir.path))) {
>>    return;
>>  }

>>  let iterator;

Comment 6

[Frank-Rainer Grahl (:frg)]

Attachment: Check if directory exists before cleaning

I am unable to reproduce the problem with my current or a new Seamonkey profile. Maybe some other bug fixed it? The patch should be formally ok but I was unable to test it.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8719153 [details] [diff] [review]
Check if directory exists before cleaning

Review of attachment 8719153 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +7979,5 @@
>      try {
>        iterator = new OS.File.DirectoryIterator(this._baseDir.path);
>      }
>      catch (e) {
> +      logger.error("Failed to clean updated system add-ons directories. No Iterator", e);

The exception will tell us where the error occurred so no need to change the message here.

Comment 8

[Frank-Rainer Grahl (:frg)]

Attachment: Check if directory exists before cleaning

Slightly modified V2 patch without error message change as suggested. 
Review+ carried forward by Dave Townsend.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/49cd9160c34b

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49cd9160c34b

Comment 11

[Kent James (:rkent)]

Pushed http://hg.mozilla.org/releases/mozilla-esr45/rev/dff6f1fadfd3 to THUNDERBIRD_45_VERBRANCH

Because we intend to do a beta on this repo as a release candidate, this is a less risky push to esr45 for this cycle.