Closed Bug 1246614 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed
thunderbird_esr45 + fixed

People

(Reporter: frg, Assigned: frg)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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
Blocks: 1246486
Summary: Clean system add ons logs error if feature directory is not in profile → Clean system add-ons logs error if feature directory is not in profile
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
Assignee: nobody → frgrahl
Attachment #8716914 - Flags: review?(dtownsend)
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.
Attachment #8716914 - Flags: review?(dtownsend)
Dave,

 not even logging an info message when the directory is not there?
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
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;
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.
Attachment #8719153 - Flags: review?(dtownsend)
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.
Attachment #8719153 - Flags: review?(dtownsend) → review+
Slightly modified V2 patch without error message change as suggested. 
Review+ carried forward by Dave Townsend.
Attachment #8719153 - Attachment is obsolete: true
Attachment #8721777 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/49cd9160c34b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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.
You need to log in before you can comment on or make changes to this bug.