1052734 - nsSearchService.js:38 - octal literals and octal escape sequences are deprecated

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Daniel Holbert [:dholbert]]

Noticed these JS warnings at startup of my debug build, with a fresh profile:
{
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:38 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:38 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:38 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:39 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:39 - octal literals and octal escape sequences are deprecated
System JS : WARNING file:///$OBJDIR/dist/bin/components/nsSearchService.js:39 - octal literals and octal escape sequences are deprecated
}

(Not sure why I get 3 copies of each warning, but I do.)

This is from these lines:
> const PERMS_FILE      = 0644;
> const PERMS_DIRECTORY = 0755;
which date back to bug 317107.

(And for reference, we disallowed octal literals in strict-mode JS in bug 514559.)

Comment 1

[Daniel Holbert [:dholbert]]

It looks like PERMS_DIRECTORY is completely unused in nsSearchService.js, so we can presumably just drop that line entirely.

For PERMS_FILE, we probably want something like this chunk from NativeApp.jsm:
> 32 // 0644
> 33 const PERMS_FILE = OS.Constants.libc.S_IRUSR | OS.Constants.libc.S_IWUSR |
> 34                    OS.Constants.libc.S_IRGRP |
> 35                    OS.Constants.libc.S_IROTH;

http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#32

Comment 2

[Matthew N. [:MattN]]

Lines to change: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?rev=30445fb9d44f#40

Comment 3

[[e-mail]]

(In reply to Daniel Holbert from comment #1)
> It looks like PERMS_DIRECTORY is completely unused in nsSearchService.js, so
> we can presumably just drop that line entirely.
> 
> For PERMS_FILE, we probably want something like this chunk from NativeApp.jsm:
> > 32 // 0644
> > 33 const PERMS_FILE = OS.Constants.libc.S_IRUSR | OS.Constants.libc.S_IWUSR |
> > 34                    OS.Constants.libc.S_IRGRP |
> > 35                    OS.Constants.libc.S_IROTH;
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#32

Does that work on Windows? Probably just easier to drop the "o" into the constant. (And change PERMS_FILE to 0o666 while we're about it; let the umask do its job.)

Comment 4

[Daniel Holbert [:dholbert]]

(I have no idea. But if it doesn't, NativeApp.jsm might need a tweak.)

Comment 5

[Tomasz Kołodziejski [:tomasz]]

I wanted to work on this bug as my first bug and here's what I found:

* indeed PERMS_DIRECTORY is not used so we can just drop it
* we have two methods to refer to octal 644:
  - http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/FileUtils.jsm#26
  - http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#33

For me 0o644 seems like the way to go.

Comment 6

[Tomasz Kołodziejski [:tomasz]]

Attachment: octal-patch.diff

This is my first (omg 2 lines!) bug attempt so please be gentle.

I generated this patch from git (git patch-format) and I'm not 100% sure that it's in the right format so please have a careful look at it.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8471926 [details] [diff] [review]
octal-patch.diff

I would prefer the NativeApp.jsm approach since it gets rid of the magic number.

I was in the middle of commenting when you were working on the patch.

Comment 8

[Matthew N. [:MattN]]

Also, please include more lines of context in future patches. We recommend eight. Also see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#I%27m_all_used_to_git.2C_but_how_can_I_provide_Mercurial-ready_patches_.3F

Comment 9

[Matthew N. [:MattN]]

Actually, we are already importing FileUtils.jsm so you can just update the const to reference FileUtils.PERMS_FILE.

Comment 10

[Tomasz Kołodziejski [:tomasz]]

Attachment: octal-patch-v1.diff

I followed your suggestions. And I converted the patch to hg style using moz-git-tools. Please have a look.

Comment 11

[Matthew N. [:MattN]]

Thanks! This is a pretty simple rename so I don't think a Try push is worth the resources.

Comment 12

[Daniel Holbert [:dholbert]]

Note: Commit message needs the bug number and have "r=MattN" included before this can land.

Also, RE Try & checkin-needed -- last I heard [1], sheriffs are now *requiring* a Try push of some sort before landing checkin-needed patches.  This is to avoid the scenario where they push 10 patches, some of which lack a try push because they're totally-safe, and then that push causes bustage, and then the sheriff has to do non-trivial sleuthing to figure out which patch(es) to backout.

If you really think a Try push would be wasteful, that's fine (and I think I agree) -- but that just means someone non-sherriffy should land this. (e.g. you or me).

I nominate MattN since he's the reviewer/mentor. :D

[1] https://groups.google.com/d/msg/mozilla.dev.platform/9w0-Bh_3vVI/xWebYK64GdwJ

Comment 13

[Matthew N. [:MattN]]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Note: Commit message needs the bug number and have "r=MattN" included before
> this can land.

Good catch, I forgot to check that but thought the docs would explain it.

> Also, RE Try & checkin-needed -- last I heard [1], sheriffs are now
> *requiring* a Try push of some sort before landing checkin-needed patches. 
> This is to avoid the scenario where they push 10 patches, some of which lack
> a try push because they're totally-safe, and then that push causes bustage,
> and then the sheriff has to do non-trivial sleuthing to figure out which
> patch(es) to backout.

I've had success recently commenting that I don't think Try is necessary on some patches.

> If you really think a Try push would be wasteful, that's fine (and I think I
> agree) -- but that just means someone non-sherriffy should land this. (e.g.
> you or me).
> 
> I nominate MattN since he's the reviewer/mentor. :D

Luckily I just got r+ in the meantime on another bug so I'll push this when those Try results are green.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I'm generally willing to take a dev's word for it if they say it isn't required, but in general I get nervous about patches that aren't NPOTB or CSS/HTML-only type changes.

Comment 15

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/d178c0b658c5

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d178c0b658c5