Closed
Bug 1028983
Opened 10 years ago
Closed 10 years ago
Use OS.File to set file permissions in the webapp installer
Categories
(Firefox Graveyard :: Web Apps, defect, P3)
Firefox Graveyard
Web Apps
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: marco, Assigned: darkowlzz, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 5 obsolete files)
8.74 KB,
patch
|
Details | Diff | Splinter Review |
We should use the new API introduced in bug 1001849 to set the file permissions in the webapps installer code (in toolkit/webapps). Currently we're using nsIFile: http://mxr.mozilla.org/mozilla-central/search?string=permissions&find=%2Ftoolkit%2Fwebapps%2F&tree=mozilla-central
Updated•10 years ago
|
OS: Linux → All
Priority: -- → P3
Hardware: x86_64 → All
Reporter | ||
Updated•10 years ago
|
Whiteboard: [good first bug] → [good first bug][lang=js]
Comment 1•10 years ago
|
||
i would like to patch this bug .. please help me work on it as im new to all this!! ty
Reporter | ||
Comment 2•10 years ago
|
||
Hey Abhirav, have you already set up a Firefox building environment? If not, follow the steps outlined here: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build Once you have managed to build, you'll need to modify the two files here (http://mxr.mozilla.org/mozilla-central/search?string=permissions&find=%2Ftoolkit%2Fwebapps%2F&tree=mozilla-central) to use the API introduced in bug 1001849 (unfortunately still undocumented on MDN) instead of nsIFile. If you need more info, feel free to ping me on IRC (I'm marco on #openwebapps). P.S.: The API is not documented on MDN, but you should be able to understand how to use it by looking at its tests here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/tests/xpcshell/test_osfile_async_setPermissions.js
Assignee | ||
Comment 3•10 years ago
|
||
Submitting this patch. Didn't see any activity by Abhirav for the past 1 week. I'm fine if he comes back and takes on this bug. Ran mochitests on toolkit/webapps/ , no failures. Please review the patch and let me know if anything else if required.
Attachment #8484856 -
Flags: feedback?(mar.castelluccio)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8484856 [details] [diff] [review] bug1028983.patch Review of attachment 8484856 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Sorry for the delay in the review! It would be great if you could write a test to verify that this works correctly (and to avoid potential regressions). You could extend these two tests to check the permissions of the affected files: http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/tests/test_hosted.xul http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/tests/test_packaged.xul
Attachment #8484856 -
Flags: feedback?(mar.castelluccio) → feedback+
Assignee | ||
Comment 5•10 years ago
|
||
Added tests for the configuration files, checking their file permissions. Let me know if the tests are correct.
Attachment #8484856 -
Attachment is obsolete: true
Attachment #8490869 -
Flags: feedback?(mar.castelluccio)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8490869 [details] [diff] [review] Set file persmissions using OS.File with tests. Review of attachment 8490869 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/webapps/tests/test_hosted.xul @@ +106,5 @@ > yield wait(1000); > } > ok(true, "App launchable"); > + > + stat = yield OS.File.stat(testAppInfo.webappINI); You should test the permissions of this file only on Mac and the permissions of the desktopINI file only on Linux. You could do something like this: > if (MAC) { > // Test testAppInfo.webappINI > } else if (LINUX) { > // Test testAppInfo.desktopINI (you'd need to add this property to the TestAppInfo object in http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/tests/head.js) > } Nit: I'd use octal values instead of decimal, it's clearer. The same comments apply to test_packaged.xul.
Attachment #8490869 -
Flags: feedback?(mar.castelluccio) → feedback+
Assignee | ||
Comment 7•10 years ago
|
||
Update the test. Thanks for the pointers to correct the tests :)
Attachment #8490869 -
Attachment is obsolete: true
Attachment #8492019 -
Flags: feedback?(mar.castelluccio)
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8492019 [details] [diff] [review] Set file persmissions using OS.File with tests. Review of attachment 8492019 [details] [diff] [review]: ----------------------------------------------------------------- One last bit missing, thank you for the patch! Do you have try access? (https://wiki.mozilla.org/ReleaseEngineering/TryServer) ::: toolkit/webapps/tests/test_hosted.xul @@ +111,5 @@ > + stat = yield OS.File.stat(testAppInfo.webappINI); > + is(stat.unixMode, 0o644, "Configuration file persmissions correct"); > + } else if (LINUX) { > + stat = yield OS.File.stat(testAppInfo.desktopINI); > + is(stat.unixMode, 0o644, "Configuration file persmissions correct"); This should be 0o744 (because the PERMS_FILE value is ORed with S_IXUSR). Nit: s/persmissions/permissions :D ::: toolkit/webapps/tests/test_packaged.xul @@ +124,5 @@ > + stat = yield OS.File.stat(testAppInfo.webappINI); > + is(stat.unixMode, 0o644, "Configuration file persmissions correct"); > + } else if (LINUX) { > + stat = yield OS.File.stat(testAppInfo.desktopINI); > + is(stat.unixMode, 0o644, "Configuration file persmissions correct"); Idem.
Attachment #8492019 -
Flags: feedback?(mar.castelluccio) → feedback+
Assignee | ||
Comment 9•10 years ago
|
||
Sorry for the long delay. Made the changes and pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b7f27d8e83d3
Attachment #8492019 -
Attachment is obsolete: true
Attachment #8502379 -
Flags: feedback?(mar.castelluccio)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8502379 [details] [diff] [review] Set file persmissions using OS.File with tests. Review of attachment 8502379 [details] [diff] [review]: ----------------------------------------------------------------- No worries! Looks like the tests are failing on Linux. The problem is that the function in which you're calling OS.File.setPermissions is not a "task", so you can't use yield in that way (if you're curious, here's the Task.jsm documentation: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm). The fix is actually pretty simple, you just need to "wrap" the method with Task.async (http://mykzilla.blogspot.com/2014/03/simplify-asynchronous-method.html) So: _createSystemFiles: function(aInstallDir) { ... }, would become: _createSystemFiles: Task.async(function*(aInstallDir) { }), After this change, the callers of _createSystemFiles will need to call it using yield: yield _createSystemFiles(...);
Attachment #8502379 -
Flags: feedback?(mar.castelluccio) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Made the changes and pushed to try. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2ce37207413d
Attachment #8502379 -
Attachment is obsolete: true
Attachment #8503037 -
Flags: feedback?(mar.castelluccio)
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8503037 [details] [diff] [review] Set file persmissions using OS.File with tests. Review of attachment 8503037 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Please send this to try again before requesting a checkin, to make sure tests are passing on Linux now.
Attachment #8503037 -
Flags: feedback?(mar.castelluccio) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Pushed again https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=810bda6d908c
Assignee: nobody → indiasuny000
Attachment #8503037 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Reporter | ||
Comment 14•10 years ago
|
||
Thank you! Tests are passing now, so you can request a checkin (you just need to set "checkin-needed" in the Keywords)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/400b8c7a7f01
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/400b8c7a7f01
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 36
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•