⚙ D99729 Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm and respect umask in IOUtils::SetPermissions

emalysz added inline comments.Dec 17 2020, 5:40 PM

toolkit/components/downloads/test/unit/common_test_Download.js
340	@barret Rerequesting review for my changes to this test file. This comment here suggests that we do honor the umask value: https://searchfox.org/mozilla-central/rev/8d722de75886d6bffc116772a1db8854e34ee6a7/toolkit/components/downloads/DownloadIntegration.jsm#670, but I'm not sure we're doing that. The permissions can either be 0o400 or 0o666, so I'm not sure why this value is included in the test

emalysz updated this revision to Diff 382922.Jan 5 2021, 3:53 PM

Harbormaster completed remote builds in B273806: Diff 382922.Jan 5 2021, 3:53 PM

barret requested changes to this revision.Jan 12 2021, 6:46 PM

barret added inline comments.

toolkit/components/downloads/test/unit/common_test_Download.js
340	The old version did respect the umask, the new one does not. IOUtils implementation uses nsIFile::SetPermissions() which does not respect umask: https://searchfox.org/mozilla-central/rev/8f4f3e6fea43f5920f05907e5e477bcd04d1134e/xpcom/io/nsLocalFileUnix.cpp#1111 OS.File.setPermissions does respect umask: https://searchfox.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_unix_front.jsm#216 We should probably add an option to setPermissions to respect umask, defaulting to true

This revision now requires changes to proceed.Jan 12 2021, 6:46 PM

emalysz updated this revision to Diff 386048.Jan 13 2021, 11:22 PM

Harbormaster completed remote builds in B276431: Diff 386048.Jan 13 2021, 11:22 PM

emalysz updated this revision to Diff 386049.Jan 13 2021, 11:23 PM

emalysz retitled this revision from Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm to Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm and respect umask in IOUtils::SetPermissions.

emalysz marked an inline comment as done.

Harbormaster completed remote builds in B276432: Diff 386049.Jan 13 2021, 11:23 PM

Code analysis found 1 defect in the diff 386048:

1 defect found by eslint (Mozlint)

You can run this analysis locally with:

./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Code analysis found 1 defect in the diff 386049:

1 defect found by eslint (Mozlint)

You can run this analysis locally with:

./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

emalysz updated this revision to Diff 386090.Jan 14 2021, 12:07 AM

Harbormaster completed remote builds in B276449: Diff 386090.Jan 14 2021, 12:07 AM

barret requested changes to this revision.Jan 14 2021, 12:19 AM

barret added inline comments.

dom/system/tests/ioutils/test_ioutils_set_permissions.html
38	Why aren't we checking against the system umask here?
47–48	nit: blank line between these
59	And here?

This revision now requires changes to proceed.Jan 14 2021, 12:19 AM

emalysz updated this revision to Diff 386102.Jan 14 2021, 12:52 AM

emalysz marked 3 inline comments as done.

Harbormaster completed remote builds in B276460: Diff 386102.Jan 14 2021, 12:52 AM

barret accepted this revision.Jan 14 2021, 7:05 PM

This revision is now accepted and ready to land.Jan 14 2021, 7:05 PM

Closed by commit rMOZILLACENTRAL43f2f5614583: Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm and respect… (authored by emalysz). · Explain WhyJan 15 2021, 3:05 AM

This revision was automatically updated to reflect the committed changes.

emalysz added a commit: rMOZILLACENTRAL43f2f5614583: Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm and respect….

NarcisB reopened this revision.Jan 15 2021, 3:42 AM

This revision is now accepted and ready to land.Jan 15 2021, 3:42 AM

NarcisB added a reverting change: rMOZILLACENTRAL73234f999362: Backed out changeset 43f2f5614583 (bug 1649611) for bustages on IOUtils.cpp….Jan 15 2021, 3:42 AM

emalysz updated this revision to Diff 387240.Jan 15 2021, 7:33 PM

Harbormaster completed remote builds in B277340: Diff 387240.Jan 15 2021, 7:33 PM

emalysz updated this revision to Diff 387362.Jan 15 2021, 9:42 PM

Harbormaster completed remote builds in B277464: Diff 387362.Jan 15 2021, 9:42 PM

Closed by commit rMOZILLACENTRAL4934ec1fbb6f: Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm and respect… (authored by emalysz). · Explain WhyJan 16 2021, 12:40 AM

This revision was automatically updated to reflect the committed changes.

emalysz added a commit: rMOZILLACENTRAL4934ec1fbb6f: Bug 1649611: Remove OS.File usage from DownloadIntegration.jsm and respect….

malexandru reopened this revision.Jan 16 2021, 10:52 AM

This revision is now accepted and ready to land.Jan 16 2021, 10:52 AM

malexandru added a reverting change: rMOZILLACENTRAL33f255475ce3: Backed out changeset 4934ec1fbb6f (bug 1649611) for causing xpcshell failures….Jan 16 2021, 10:52 AM

emalysz updated this revision to Diff 391515.Jan 27 2021, 4:14 PM

Harbormaster completed remote builds in B281074: Diff 391515.Jan 27 2021, 4:14 PM

emalysz updated this revision to Diff 391516.Jan 27 2021, 4:16 PM

Harbormaster completed remote builds in B281075: Diff 391516.Jan 27 2021, 4:16 PM

Hey @barret ! Missed this failure before - the windows zone information relies on winAllowLengthBeyondMaxPathWithCaveats. DownloadIntegration seems to be the only place that uses that arg, but I figured it may make more sense too add this to PathUtils. Requesting review for those changes :)

barret accepted this revision.Jan 27 2021, 6:24 PM

barret added inline comments.

dom/chrome-webidl/PathUtils.webidl
68	This should be called "toExtendedWindowsPath" or similar
dom/system/PathUtils.cpp
220	This should throw on non-Windows platforms.
236	"uncPath"
237	"normalPath" or "nonUNCPath"