684121 - Don't access Services.appinfo at import time

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

It breaks 'make package' (bug 684095) but probably it's also not a good idea in general. For instance, our xpcshell test harness registers a fake nsIXULAppInfo service because of that, even though only a small fraction of our tests actually require it.

Comment 1

[Philipp von Weitershausen [:philikon]]

I can find two places:

* the prefs engine (PREFS_GUID). This can be converted to a lazy getter on the global object.
* AsyncResource and SyncStorageRequest (_userAgent). This can be converted to a lazy getter on the respective prototypes.
* util.js (_sessionCID): this can be a lazy getter on the global object, or we just make the Svc.Form getter explicitly access Services.appinfo, or we actually move the Svc.Form lazy getter to the tab engine because it's the only place where it's used. (Die, Svc, die!)

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> I can find two places:

...

Comment 3

[Iskandar Rafikov]

I want to work on this bug

Comment 4

[Richard Newman [:rnewman]]

(In reply to Iskandar Rafikov from comment #3)
> I want to work on this bug

Great! Comment 1 has pretty much everything you need, after you run through the steps here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions

Comment 5

[Dorel Barbu]

Hello! I would like to work on this bug. I've already solved one, and this seems a little bit more challenging. Could you give me some extra hints as to where should I start, please? I've read the already given info, but I need a little bit more.

Comment 6

[Richard Newman [:rnewman]]

Reading Bug 684095 should give valuable background.

/services/ has several instances of code (as described in Comment 1) like:

var _sessionCID = Services.appinfo.ID == SEAMONKEY_ID ?
  "@mozilla.org/suite/sessionstore;1" :
  "@mozilla.org/browser/sessionstore;1";

That means simply loading that code will cause `Services.appinfo.ID` to be evaluated, which isn't possible in all environments.

The fix is to change that code to *not* do so.

In this particular example, that probably means abandoning defineLazyServiceGetter in the code that uses `_sessionCID`, perhaps to something like this:

```
XPCOMUtils.defineLazyGetter(Svc, "Session", function () {
  let sessionCID = Services.appinfo.ID == SEAMONKEY_ID ?
                   "@mozilla.org/suite/sessionstore;1" :
                   "@mozilla.org/browser/sessionstore;1";
  return Cc[sessionCID].getService(Ci["nsISessionStore"]);
});
```

Comment 7

[Dorel Barbu]

(In reply to Richard Newman [:rnewman] from comment #6)
> Reading Bug 684095 should give valuable background.
> 
> /services/ has several instances of code (as described in Comment 1) like:
> 
> var _sessionCID = Services.appinfo.ID == SEAMONKEY_ID ?
>   "@mozilla.org/suite/sessionstore;1" :
>   "@mozilla.org/browser/sessionstore;1";
> 
> That means simply loading that code will cause `Services.appinfo.ID` to be
> evaluated, which isn't possible in all environments.
> 
> The fix is to change that code to *not* do so.
> 
> In this particular example, that probably means abandoning
> defineLazyServiceGetter in the code that uses `_sessionCID`, perhaps to
> something like this:
> 
> ```
> XPCOMUtils.defineLazyGetter(Svc, "Session", function () {
>   let sessionCID = Services.appinfo.ID == SEAMONKEY_ID ?
>                    "@mozilla.org/suite/sessionstore;1" :
>                    "@mozilla.org/browser/sessionstore;1";
>   return Cc[sessionCID].getService(Ci["nsISessionStore"]);
> });
> ```

All right. This would be my first backend bug. My question is how cam I verify that Services.appinfo is accessed or not at import tine? How should I check this? Also, could you assign me, please?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Dorel Barbu from comment #7)
> My question is how cam I verify that Services.appinfo is accessed or not at import tine?

If it's referenced in the top-level environment of the JavaScript file, rather than being inside a function or closure.

> Also, could you assign me, please?

We assign bugs to new contributors when you have a patch ready to upload; ASSIGNED means you're really on the hook for getting the bug finished and addressing comments in a timely fashion, and there's usually some churn before getting to that point.

Comment 9

[Dorel Barbu]

(In reply to Richard Newman [:rnewman] from comment #8)
> (In reply to Dorel Barbu from comment #7)
> > My question is how cam I verify that Services.appinfo is accessed or not at import tine?
> 
> If it's referenced in the top-level environment of the JavaScript file,
> rather than being inside a function or closure.
> 
> > Also, could you assign me, please?
> 
> We assign bugs to new contributors when you have a patch ready to upload;
> ASSIGNED means you're really on the hook for getting the bug finished and
> addressing comments in a timely fashion, and there's usually some churn
> before getting to that point.

Ok, then. I'll get to work and post here when I have a patch ready.

Comment 10

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Richard Newman [:rnewman] from comment #6)
> Reading Bug 684095 should give valuable background.
> 
> /services/ has several instances of code (as described in Comment 1) like:
> 
> var _sessionCID = Services.appinfo.ID == SEAMONKEY_ID ?
>   "@mozilla.org/suite/sessionstore;1" :
>   "@mozilla.org/browser/sessionstore;1";
> 
> That means simply loading that code will cause `Services.appinfo.ID` to be
> evaluated, which isn't possible in all environments.
> 
> The fix is to change that code to *not* do so.
> 
> In this particular example, that probably means abandoning
> defineLazyServiceGetter in the code that uses `_sessionCID`, perhaps to
> something like this:
> 
> ```
> XPCOMUtils.defineLazyGetter(Svc, "Session", function () {
>   let sessionCID = Services.appinfo.ID == SEAMONKEY_ID ?
>                    "@mozilla.org/suite/sessionstore;1" :
>                    "@mozilla.org/browser/sessionstore;1";
>   return Cc[sessionCID].getService(Ci["nsISessionStore"]);
> });
> ```
Hello. Did a quick search for `Services.appinfo.ID` and got this: http://searchfox.org/mozilla-central/search?q=Services.appinfo.ID&case=true&regexp=false&path=services%2F

I didn't see the `var _sessionCID` bit in the search results. How would I go about solving the bug?

Comment 11

[Richard Newman [:rnewman]]

It looks like that line of code was removed quite recently, in Bug 1358648:

https://hg.mozilla.org/mozilla-central/rev/dff6d3f923d7#l4.117

There's another use from Comment 1 that's still around:

http://searchfox.org/mozilla-central/source/services/sync/modules/engines/prefs.js#25

Comment 12

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> I can find two places:
> 
> * the prefs engine (PREFS_GUID). This can be converted to a lazy getter on
> the global object.
How would I go about doing this? Looking at the other bit of code referenced for `_userAgent`, I didn't think the approach could be replicated for `PREFS_GUID`.

Comment 13

[Leni Kadali [:leni1] [GMT+3]]

The answer is more or less the code below:

XPCOMUtils.defineLazyGetter(this, "PREFS_GUID", () => CommonUtils.encodeBase64URL(Services.appinfo.ID))

Discussed in #sync IRC channel

Comment 14

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Don't access Services.appinfo at import time

As stated by Philipp, the prefs engine (PREFS_GUID) accesses Services.appinfo at import time.
Changed code to use a lazy getter on the global object instead.

changed services/sync/modules/engines/prefs.js

Comment 15

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8896677 [details] [diff] [review]
Don't access Services.appinfo at import time

(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> I can find two places:
> * AsyncResource and SyncStorageRequest (_userAgent). This can be converted
> to a lazy getter on the respective prototypes.
Is this bit still needed?

Comment 16

[Mark Hammond [:markh] [:mhammond] - PTO until April 20]

Comment on attachment 8896677 [details] [diff] [review]
Don't access Services.appinfo at import time

LGTM - are you able to push to mozreview?

Comment 17

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Bug 684121 - Don't access Services.appinfo at import time

As stated by Philipp, the prefs engine (PREFS_GUID) accesses Services.appinfo at import time.
Changed code to use a lazy getter on the global object instead.

changed services/sync/modules/engines/prefs.js

Review commit: https://reviewboard.mozilla.org/r/168078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168078/

Comment 18

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Mark Hammond [:markh] from comment #16)
> Comment on attachment 8896677 [details] [diff] [review]
> Don't access Services.appinfo at import time
> 
> LGTM - are you able to push to mozreview?

Yes. Push done. :-)

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8896792 [details]
Bug 684121 - Don't access Services.appinfo at import time

https://reviewboard.mozilla.org/r/168078/#review173548

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1239495d7a60
Don't access Services.appinfo at import time r=kitcambridge

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1239495d7a60