Incorrect isLinux check for gconf is spreading across the tree

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: benjamin, Assigned: hiro)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(9 attachments, 7 obsolete attachments)

4.26 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
822 bytes, patch
jimm
: review+
Details | Diff | Splinter Review
5.22 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
1.01 KB, patch
seth
: review+
Details | Diff | Splinter Review
5.01 KB, patch
hiro
: review+
Details | Diff | Splinter Review
2.51 KB, patch
hiro
: review+
Details | Diff | Splinter Review
6.42 KB, patch
hiro
: review+
Details | Diff | Splinter Review
2.46 KB, patch
hiro
: review+
Details | Diff | Splinter Review
1.87 KB, patch
hiro
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Spun off from bug 1167201. We have a pattern that's creeping around the tree of checking for Linux like this:

let isLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc);

That's not smart: distros can (and some do) explicitly --disable-gconf, but even if they don't, gconf can be not present at runtime.

Glandium says: "@mozilla.org/gio-service;1 would be a better one, as it's always there."

Ted says: "As to the isLinux thing, we should fix bug 1150818 and let tests use mozinfo instead of horrible checks like that. :-/"
(Assignee)

Comment 1

4 years ago
I take this.
Assignee: nobody → hiikezoe
(Assignee)

Comment 2

3 years ago
Created attachment 8610919 [details] [diff] [review]
Use mozinfo to identify platform etc. in xpcshell tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30726143e688

I will request a review once bug 1150818 is fixed.
(Assignee)

Comment 3

3 years ago
Created attachment 8652575 [details] [diff] [review]
Part 1: Use mozinfo in dom/

This patch removed nsILocalFileOS2 in test_xml_serializer.js because I think we no longer support OS2.
Attachment #8610919 - Attachment is obsolete: true
Attachment #8652575 - Flags: review?(mrbkap)
(Assignee)

Comment 4

3 years ago
Created attachment 8652577 [details] [diff] [review]
Part 2: Use mozinfo in modules/libmar/.
Attachment #8652577 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 5

3 years ago
Created attachment 8652580 [details] [diff] [review]
Part 3: Use mozinfo in xpcom/.
Attachment #8652580 - Flags: review?(benjamin)
(Assignee)

Comment 6

3 years ago
Created attachment 8652581 [details] [diff] [review]
Part 4: Use mozinfo in uriloader/.
Attachment #8652581 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 7

3 years ago
Created attachment 8652584 [details] [diff] [review]
Part 5: Use mozinfo in widget/.
Attachment #8652584 - Flags: review?(jmathies)
(Assignee)

Comment 8

3 years ago
Created attachment 8652586 [details] [diff] [review]
Part 6: Use mozinfo in security/.
Attachment #8652586 - Flags: review?(cmanchester)
(Assignee)

Comment 9

3 years ago
Created attachment 8652587 [details] [diff] [review]
Part 7: Use mozinfo in netwerk/.

I should have integrated this patch in Part 4.
Attachment #8652587 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 10

3 years ago
Created attachment 8652588 [details] [diff] [review]
Part 8: Use mozinfo in image/.
Attachment #8652588 - Flags: review?(tnikkel)
Comment on attachment 8652586 [details] [diff] [review]
Part 6: Use mozinfo in security/.

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

Sorry, I'm not a reviewer here.
Attachment #8652586 - Flags: review?(cmanchester)
(Assignee)

Comment 12

3 years ago
Created attachment 8652592 [details] [diff] [review]
Part 9: Use mozinfo in services/.
Attachment #8652592 - Flags: review?(rnewman)
Attachment #8652588 - Flags: review?(tnikkel) → review?(seth)
Comment on attachment 8652592 [details] [diff] [review]
Part 9: Use mozinfo in services/.

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

Where's "android"? You're going to cause the mocked XULAppInfo to fall back to thinking it's xpcshell instead of Linux, which will probably confuse code elsewhere. That code probably isn't covered adequately by tests, so be careful.

If this is the pattern you've taken elsewhere in these patches (I haven't checked) you should spend the time to make sure that Android and Firefox OS work correctly.
Attachment #8652592 - Flags: review?(rnewman) → review-
(Assignee)

Comment 14

3 years ago
Created attachment 8652638 [details] [diff] [review]
Part 9: Use mozinfo in services/. v2

Richard, thanks for making me realize the mistake!

This patch does just replace |"@mozilla.org/windows-registry-key;1" in Cc| and |"nsILocalFileMac" in Ci| to |mozinfo == ".."| to avoid such mistakes.
Attachment #8652592 - Attachment is obsolete: true
Attachment #8652638 - Flags: review?(rnewman)
Comment on attachment 8652588 [details] [diff] [review]
Part 8: Use mozinfo in image/.

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

Looks good!

::: image/test/unit/test_imgtools.js
@@ +129,5 @@
>  // +/- 1 value compared to other platforms, so we need to compare against a
>  // different set of reference images. nsIXULRuntime.OS doesn't seem to be
>  // available in xpcshell, so we'll use this as a kludgy way to figure out if
>  // we're running on Windows.
> +var isWindows = mozinfo.os == "win";

You should remove "nsIXULRuntime.OS doesn't seem to be available in xpcshell, so we'll use this as a kludgy way to figure out if we're running on Windows." from the comment above this line.
Attachment #8652588 - Flags: review?(seth) → review+
Comment on attachment 8652638 [details] [diff] [review]
Part 9: Use mozinfo in services/. v2

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

Looks safe to me!
Attachment #8652638 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED

Updated

3 years ago
Attachment #8652584 - Flags: review?(jmathies) → review+
Attachment #8652575 - Flags: review?(mrbkap) → review+
Attachment #8652577 - Flags: review?(robert.strong.bugs) → review+

Updated

3 years ago
Attachment #8652581 - Flags: review?(jduell.mcbugs) → review+

Updated

3 years ago
Attachment #8652587 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 17

3 years ago
Comment on attachment 8652586 [details] [diff] [review]
Part 6: Use mozinfo in security/.

dkeeler, can you please take a look this?
Attachment #8652586 - Flags: review?(dkeeler)
Comment on attachment 8652586 [details] [diff] [review]
Part 6: Use mozinfo in security/.

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

Great - thanks for taking care of this.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +19,5 @@
>  // The test EV roots are only enabled in debug builds as a security measure.
>  //
>  // Bug 1008316: B2G doesn't have EV enabled, so EV is not expected even in debug
>  // builds.
>  const gEVExpected = isDebugBuild && !("@mozilla.org/b2g-process-global;1" in Cc);

Hmmm - does mozinfo expose a better way to do this? (i.e. detect if the process is b2g or not)
Attachment #8652586 - Flags: review?(dkeeler) → review+
(Reporter)

Updated

3 years ago
Attachment #8652580 - Flags: review?(benjamin) → review+
(Assignee)

Comment 19

3 years ago
Created attachment 8672455 [details] [diff] [review]
Part 1: Use mozinfo in dom/ v2

Rebased.
Attachment #8652575 - Attachment is obsolete: true
Attachment #8672455 - Flags: review+
(Assignee)

Comment 20

3 years ago
Created attachment 8672456 [details] [diff] [review]
Part 2: Use mozinfo in modules/libmar/ v2

Rebased.
Attachment #8652577 - Attachment is obsolete: true
Attachment #8672456 - Flags: review+
(Assignee)

Comment 21

3 years ago
Created attachment 8672457 [details] [diff] [review]
Part 3: Use mozinfo in xpcom/ v2

Rebased
Attachment #8652580 - Attachment is obsolete: true
Attachment #8672457 - Flags: review+
(Assignee)

Comment 22

3 years ago
Created attachment 8672458 [details] [diff] [review]
Part 6: Use mozinfo in security/ v2

Rebased.
Attachment #8652586 - Attachment is obsolete: true
Attachment #8672458 - Flags: review+
(Assignee)

Comment 23

3 years ago
Created attachment 8672459 [details] [diff] [review]
Part 9: Use mozinfo in services/. v3

Rebased.
Attachment #8652638 - Attachment is obsolete: true
Attachment #8672459 - Flags: review+
(Assignee)

Comment 24

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dc2cbe0ae51

Thank you all reviewers!
Keywords: checkin-needed
(Assignee)

Comment 25

3 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #18)
> Comment on attachment 8652586 [details] [diff] [review]
> Part 6: Use mozinfo in security/.
> 
> Review of attachment 8652586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great - thanks for taking care of this.
> 
> ::: security/manager/ssl/tests/unit/head_psm.js
> @@ +19,5 @@
> >  // The test EV roots are only enabled in debug builds as a security measure.
> >  //
> >  // Bug 1008316: B2G doesn't have EV enabled, so EV is not expected even in debug
> >  // builds.
> >  const gEVExpected = isDebugBuild && !("@mozilla.org/b2g-process-global;1" in Cc);
> 
> Hmmm - does mozinfo expose a better way to do this? (i.e. detect if the
> process is b2g or not)

I guess mozinfo.os =="b2g" can use for the purpose.
(Assignee)

Comment 28

3 years ago
There remains some platform checks don't use mozinfo, I'd hope someone who notice it will fix it one by one.
You need to log in before you can comment on or make changes to this bug.