652850 - SafeInstallOperation can't rollback move/rename operations

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Giorgio Maone [:ma1]]

While investigating code possibly involved with bug 644856, I've found http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#201 is probably incorrect:

this._installedFiles.push({ oldFile: null, newFile: newFile });

should be actually

this._installedFiles.push({ oldFile: oldFile, newFile: newFile });

otherwise http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#188 is meaningless and the oldFile variable useless.

The effect of the current code is that move/rename operations cannot be rolled back because the original path gets lost.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Well damn, good catch.

Going to have to think of how I can do proper testing here, simulating file move failures isn't easy though

Comment 2

[Giorgio Maone [:ma1]]

(In reply to comment #1)
> simulating file
> move failures isn't easy though

Wouldn't moving to a surely non-existent path always fail?

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #2)
> (In reply to comment #1)
> > simulating file
> > move failures isn't easy though
> 
> Wouldn't moving to a surely non-existent path always fail?

Yes, but the code always erases the target path and recreates it as it moves the files. If I do something to make the target always unwritable then we'd never get to the SafeInstallOperation code. I think I can create some fake nsIFile instances to simulate failing to move a specific file, but I need to play around with it.

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 528345 [details] [diff] [review]
patch rev 1

Might be able to test this on Windows by using a file stream.

File a followup for tests?

Comment 5

[Dave Townsend [:mossop]]

This landed yesterday: http://hg.mozilla.org/mozilla-central/rev/e4e42dac5104

Bug for tests: bug 653835

Comment 6

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

Verified fixed by check-in (merge) on aurora. I will also connect the corresponding bug for the tests and set the in-testsuite? flag, so we do not forget about it.