Page MenuHomePhabricator

Fix a crash when uploading a ZIP to Phragment
AbandonedPublic

Authored by hach-que on May 7 2014, 2:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 22, 10:32 PM
Unknown Object (File)
Sun, Dec 22, 10:32 PM
Unknown Object (File)
Sun, Dec 22, 10:32 PM
Unknown Object (File)
Sun, Dec 22, 10:32 PM
Unknown Object (File)
Tue, Dec 17, 11:15 AM
Unknown Object (File)
Thu, Dec 12, 10:52 AM
Unknown Object (File)
Tue, Dec 10, 12:56 AM
Unknown Object (File)
Sun, Dec 8, 5:33 PM

Details

Summary

This crash occurs due to a change introduced in D8671 (which isn't yet in upstream, but is still an important fix for Phragment).

Basically the code would attempt to create a directory entry twice, because there were entries like:

  • abc/def.txt

in the ZIP file, but it didn't yet have an entry for abc/. This code would add abc/ to the explicit paths to be created, but D8671 introduced a change whereby these preceding paths would automatically be created when abc/def.txt was updated or created. Because the added entry would result in an iteration like:

  • abc/def.txt
  • abc/

This would crash with a duplicate key exception, because abc/ had already been created in the abc/def.txt update.

Test Plan

This only happens the first time that a directory is missing; obviously the first call to create the directory as part of abc/def.txt in the example above will result in it not being an issue the second time around.

I've tested this change against a normal build (where no new directories were introduced) and nothing broke, so worst case scenario we're in no better position than we started with (but I am fairly confident that this is the correct fix for the issue).

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 284
Build 284: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que retitled this revision from to Fix a crash when uploading a ZIP to Phragment.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)

(Does "TBD" mean you're going to test this but haven't yet?)

hach-que added a reviewer: epriestley.

Yeah I didn't specify any reviewers in the diff because it was a draft, but then it added Blessed Reviewers anyway.

epriestley edited edge metadata.

T2543 discusses adding a formal "draft" state.

This revision is now accepted and ready to land.May 7 2014, 2:31 PM
hach-que edited edge metadata.

The proper actual fix. Depends on D8671.

hach-que edited the test plan for this revision. (Show Details)
hach-que requested a review of this revision.May 8 2014, 5:51 AM
hach-que edited edge metadata.

Putting this back into review as it's dependency isn't accepted yet (and this isn't likely to be reviewed for a while).

Merged this into D8671 to make it easier to manage (given that it depends on changes in D8671).