Closed Bug 757351 Opened 12 years ago Closed 11 years ago

Sync uses deprecated octal literals

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: gps, Assigned: AMoz)

References

(Depends on 1 open bug)

Details

(Whiteboard: [qa+])

Attachments

(1 file, 1 obsolete file)

Now seeing this message when Sync JS is loaded:

> JavaScript warning: chrome://browser/content/sync/utils.js, line 200: octal literals and octal escape sequences are deprecated

This is in passwordSave(), where the octal literal is for file permissions. Should be simple enough to fix.

File in question is browser/base/content/sync/utils.js.
Not sure if it can be useful, but chrome workers now have access to a global object OS.Constants.libc that defines S_IRGRP, S_IROTH, S_IRUSR, S_IRWXG, S_IRWXO, S_IRWXU, S_IWGRP, S_IWOTH, S_IWUSR, S_IXOTH, S_IXGRP, S_IXUSR.
Yoric: do you have an MDN link for OS.Constants.libc? I can't seem to find it...
Here is the source: http://dxr.lanedo.com/mozilla-central/dom/system/OSFileConstants.cpp.html.

But, it doesn't appear that OS is defined in the main chrome thread (opened web console from about:about). So, I don't think Sync will be able to use these constants :(
No MDN doc yet. And yes, it is defined only in worker threads. I will add support for main thread once I have started working on main-thread OS.File, but for the moment, it was deemed too early.

There is a bug for exporting it to the main thread, though (bug 750178). If you need these constants in the main thread, though, I can reprioritize that bug.
OK. Let's wait until bug 750178 is complete and then we'll tackle this. Removing mentored bug flags until we are unblocked.
Depends on: 750178
Whiteboard: [good first bug][mentor=gps][lang=js]
Also please note that FileUtils defines these constants already: https://developer.mozilla.org/en/JavaScript_code_modules/FileUtils.jsm#Constants. I don't think there's much need to duplicate this again in XPCOM/JS land, unless there's a good reason not to use those.
Maybe we're conflating two separate systems. FileUtils provides convenient and canonical ways to do File I/O with XPCOM. Yoric's stuff seems different from that. Since browser/base/content/sync/utils.js uses the XPCOM way, I think it should just use FileUtils, unless the proposal is to convert it to Yoric's system.
Indeed, looks like FileUtils is the right way to go. My work on OS.File (and OSFileConstants.cpp) is designed to eventually render FileUtils obsolete, but this might take some time.
Yoric: What's the state of OS.File these days?
sync triage: We think this should wait on yoric's work. Yoric, if that is still going, what's the bug to depend on?
Splendid!

This should be relatively easy to tackle. The offending file in the source tree is browser/base/content/sync/utils.js.
Whiteboard: [mentor=gps][lang=js]
I'll tackle this if nobody minds. I'm new here, and if I'm doing something wrong, please tell me!
Assignee: nobody → epiary
(In reply to epiary from comment #13)
> I'll tackle this if nobody minds. I'm new here, and if I'm doing something
> wrong, please tell me!

Fantastic. Welcome! You're doing everything right so far :)

You'll want to read this, at the very least, and probably more besides if you're not already familiar with contributing to the Mozilla codebase.

https://developer.mozilla.org/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

More:

https://developer.mozilla.org/docs/Developer_Guide

Either :gps or myself (:rnewman) is a great value to put in the reviewer field when you get that far, and we're here to offer advice if you need it. You can also ping one of us in #sync on IRC.

Keep us posted of your progress!
Heh, Josh mid-aired me.
Status: NEW → ASSIGNED
Whiteboard: [mentor=gps][lang=js] → [mentor=gps][lang=js][good first bug]
epiary: are you still working on this? If not, please update bug metadata appropriately. Thanks!
Flags: needinfo?(epiary)
Aha, whoops! Looks like I put this off for some time now and forgot about it. I'll work on it some time these next few weeks, and by the end of that period I'll either finish or unassign myself.
Flags: needinfo?(epiary)
Any news, epiary?
Flags: needinfo?(epiary)
Alas, no, sorry to disappoint. I'm going to remove my status as assignee.
Flags: needinfo?(epiary)
Status: ASSIGNED → NEW
Assignee: epiary → nobody
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Just wonder how to reproduce this warning
I would like to contribute to this bug. I read the comments but wasnt able to draw some notable idea from it. It would be great if I am given a clear picture of what the bug is.
1. On IRC, the mentor told me that the offending line is | stream.init(fp.file, -1, 0600, 0); | in utils.js
2. Do i need to study the entire code of utils.js or only some part.

Currently I have messed with my mercurial, it seems i need to pull a fresh copy. So it may take some time to start off. I will be updating the status here time to time.
(In reply to Amod [:greatwarrior] from comment #21)

> 1. On IRC, the mentor told me that the offending line is |
> stream.init(fp.file, -1, 0600, 0); | in utils.js

Yes. The issue is the "0600" on that line. 

Unfortunately, per <https://developer.mozilla.org/en-US/docs/PR_Open#Parameters> the constants you want -- (PR_IRUSR | PR_IWUSR) -- aren't available from JS.

OS.Constants exposes them (S_IRUSR | S_IWUSR), but unless this file already has access to OS.Constants, we shouldn't introduce a dependency just for this. So if OS.Constants is already in scope, great; use those. If not, introduce a constant "PERMISSIONS_RWUSR" defined as 0x180.

I'm marking this bug as a dependency of Bug 433295, which is essentially "you can't use these constants from JS". If OS.Constants is now widely available, including in this scope, perhaps we can dupe that bug to the landing bug for OS.Constants. (Yoric?)

> 2. Do i need to study the entire code of utils.js or only some part.

This should be a one-line fix, maybe two if we need to import a file.

> Currently I have messed with my mercurial, it seems i need to pull a fresh
> copy. So it may take some time to start off. I will be updating the status
> here time to time.

See Comment 14 for how to get started.
Depends on: 433295
Whiteboard: [mentor=gps][lang=js][good first bug] → [mentor=rnewman][lang=js][good first bug]
(In reply to Richard Newman [:rnewman] from comment #22)

> I'm marking this bug as a dependency of Bug 433295, which is essentially
> "you can't use these constants from JS". If OS.Constants is now widely
> available, including in this scope, perhaps we can dupe that bug to the
> landing bug for OS.Constants. (Yoric?)

On the main thread, OS.Constants is not (and probably never will be) automatically in the scope. You need to import it with
 Components.utils.import("resource:///gre/modules/osfile.jsm");

On worker threads, OS.Constants is in the default scope.
Attached patch introduced PERMISSIONS_RWUSR (obsolete) — Splinter Review
Attachment #740393 - Flags: feedback?(rnewman)
Comment on attachment 740393 [details] [diff] [review]
introduced PERMISSIONS_RWUSR

Review of attachment 740393 [details] [diff] [review]:
-----------------------------------------------------------------

Please follow these instructions to set patch headers etc., and upload an updated patch.

Thanks!

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/base/content/sync/utils.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// introduce PERMISSIONS_RWUSR as third parameter in stream.init();
> +const PERMISSIONS_RWUSR = 0x180;

Newline after this line.

I'd have the comment be:

// Equivalent to 0600 permissions; used for saved Sync Recovery Key.
// This constant can be replaced when the equivalent values are available to
// chrome JS; see Bug 433295 and Bug 757351.
Attachment #740393 - Flags: feedback?(rnewman) → feedback+
Assignee: nobody → amod.narvekar
Status: NEW → ASSIGNED
Is the header correct ? 
especially : Bug 757351 - Sync uses deprecated octal literals; r=rnewman

Since I am new here, I am not much used to patch headers. So I apologize for any mistake.
Attachment #740393 - Attachment is obsolete: true
Attachment #740409 - Flags: review?(rnewman)
Comment on attachment 740409 [details] [diff] [review]
Added patch headers and made the mentioned changes.

Review of attachment 740409 [details] [diff] [review]:
-----------------------------------------------------------------

My personal style is ". r=rnewman", but semicolon is fine :D
Attachment #740409 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/services/services-central/rev/8e70086dd006

STR: QA, please ensure that saving the Recovery Key to a file still works, leaving a file that's user RW and world unreadable.
Whiteboard: [mentor=rnewman][lang=js][good first bug] → [fixed in services][qa+]
https://hg.mozilla.org/mozilla-central/rev/8e70086dd006
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa+] → [qa+]
Target Milestone: --- → mozilla23
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.