Closed Bug 986499 Opened 6 years ago Closed 6 years ago

Avoid the Deprecated.jsm compartment when not needed


(Toolkit :: General, defect)

Not set





(Reporter: mak, Assigned: avp)


(Blocks 1 open bug)


(Whiteboard: [mentor=Yoric][lang=js])


(1 file, 4 obsolete files)

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.
Whiteboard: [mentor=Yoric][lang=js]
Hi Yoric,

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

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:

(there's probably no value in changing the browser_Deprecated.js caller since that's just a test)
Thanks Gavin, will attach a patch soon !
Assignee: nobody → abhishekp.bugzilla
Attachment #8395504 - Flags: feedback?(dteller)
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
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 .
Attachment #8395504 - Flags: feedback?(dteller) → feedback+
Attached patch Made the suggested changes (obsolete) — Splinter Review
Attachment #8395504 - Attachment is obsolete: true
Attachment #8395602 - Flags: feedback?(dteller)
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);
Attachment #8395602 - Flags: feedback?(dteller) → review+
Attached patch bug-986499-fix.diff (obsolete) — Splinter Review
I have made the changes. All the Mochitests on Toolkit directory passed, but the some xpcshell tests failed.

Test Results on Pastebin :

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?
Attachment #8395602 - Attachment is obsolete: true
Flags: needinfo?(dteller)
Sorry, I meant

let Deprecated = Cu.import(..., {}).Deprecated;
Flags: needinfo?(dteller)
Attached patch bug-986499-fix.diff (obsolete) — Splinter Review
All Tests Passed !
Attachment #8395779 - Attachment is obsolete: true
Attachment #8396336 - Flags: review?(dteller)
Comment on attachment 8396336 [details] [diff] [review]

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.
Attachment #8396336 - Flags: review?(dteller) → review+
Attachment #8396336 - Attachment is obsolete: true
Attachment #8396338 - Flags: review?(dteller)
Comment on attachment 8396338 [details] [diff] [review]

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

Looks good to me, thanks for the patch.
Attachment #8396338 - Flags: review?(dteller) → review+
Is everything ready for checkin, abhishekp?
Flags: needinfo?(abhishekp.bugzilla)
Flags: needinfo?(abhishekp.bugzilla)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.