986499 - Avoid the Deprecated.jsm compartment when not needed

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Marco Bonardo [:mak]]

As in many other cases people keeps forgetting about defineLazyModuleGetter.
Deprecated.jsm is a module that is needed only in certain cases, Cu.import-ing it doesn't make sense.
We may avoid this compartment on startup.

Comment 1

[Abhishek Potnis [:avp]]

Hi Yoric,

I would like to work on this, can you please help me get started with it?

Thanks.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Hi Abhishek!

The goal of this bug, overall, is to change any Cu.import("resource://gre/modules/Deprecated.jsm"); calls into the equivalent defineLazyModuleGetter calls, so that the module import is made lazy. Let us know if you have specific questions about how to get started.

Here's a list of affected callers:
http://mxr.mozilla.org/mozilla-central/search?string=%28%22resource%3A%2F%2Fgre%2Fmodules%2FDeprecated

(there's probably no value in changing the browser_Deprecated.js caller since that's just a test)

Comment 3

[Abhishek Potnis [:avp]]

Thanks Gavin, will attach a patch soon !

Comment 4

[Abhishek Potnis [:avp]]

Attachment: Made the calls to import Deprecated.jsm lazy

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8395504 [details] [diff] [review]
Made the calls to import Deprecated.jsm lazy

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

Good start, thanks.

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +1239,5 @@
>  };
>  
>  function warnDeprecated() {
> +  XPCOMUtils.defineLazyModuleGetter(this, 'Deprecated',
> +  'resource://gre/modules/Deprecated.jsm');

That one is not necessary, since warnDeprecated() itself is called only when needed. On the other hand, there is a small error in the original import, so let's take the opportunity to fix it.

Let's replace
Cu.import("resource://gre/modules/Deprecated.jsm");
with
let Deprecated = Cu.import("resource://gre/modules/Deprecated.jsm", {});

This will ensure that we do not pollute the global scope.

::: toolkit/mozapps/downloads/DownloadLastDir.jsm
@@ +105,5 @@
>    // This function is now deprecated as it uses the sync nsIContentPrefService
>    // interface. New consumers should use the getFileAsync function.
>    getFile: function (aURI) {
> +    XPCOMUtils.defineLazyModuleGetter(this, 'Deprecated',
> +    'resource://gre/modules/Deprecated.jsm');

As in nsContentPrefService.js .

Comment 6

[Abhishek Potnis [:avp]]

Attachment: Made the suggested changes

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8395602 [details] [diff] [review]
Made the suggested changes

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

r=me with the modification below
(assuming it passes tests)

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +25,5 @@
>  const Ci = Components.interfaces;
>  
>  let SharedAll = {};
>  Cu.import("resource://gre/modules/osfile/osfile_shared_allthreads.jsm", SharedAll);
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

, this);

Comment 8

[Abhishek Potnis [:avp]]

Attachment: bug-986499-fix.diff

I have made the changes. All the Mochitests on Toolkit directory passed, but the some xpcshell tests failed.

Test Results on Pastebin : http://pastebin.mozilla.org/4668606

The error is in /toolkit/components/contentprefs/nsContentPrefService.js
The call to Deprecated.warning() is not happening with the error "Deprecated.warning is not a function". So is the Deprecated object not being initialized before the call? How do I debug and resolve this?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Sorry, I meant

let Deprecated = Cu.import(..., {}).Deprecated;

Comment 10

[Abhishek Potnis [:avp]]

Attachment: bug-986499-fix.diff

All Tests Passed !

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8396336 [details] [diff] [review]
bug-986499-fix.diff

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

Good for me, with the change below.

::: toolkit/components/telemetry/TelemetryFile.jsm
@@ +12,5 @@
>  const Cr = Components.results;
>  const Cu = Components.utils;
>  
>  Cu.import("resource://gre/modules/Services.jsm", this);
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Please add argument |this|, for compatibility with B2G.

Comment 12

[Abhishek Potnis [:avp]]

Attachment: bug-986499-fix.diff

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8396338 [details] [diff] [review]
bug-986499-fix.diff

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

Looks good to me, thanks for the patch.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Is everything ready for checkin, abhishekp?

Comment 15

[Abhishek Potnis [:avp]]

Yes.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3f9720851c

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d3f9720851c