921157 - Deprecate FileUtils.getDir(..., ..., true) during startup/shutdown

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

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

FileUtils.getDir(..., ..., true) causes main thread I/O. That's pretty bad, especially during startup/shutdown. We should alter FileUtils.jsm to print out warnings when we use this method during startup/shutdown.

Comment 1

[Abhishek Potnis [:avp]]

Hi Yoric,
I would like to work on this bug. Could please help me get started with it?

Thanks.

Comment 2

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

To add a deprecation warning, you will need to patch this file: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/FileUtils.jsm?from=FileUtils.jsm#l62 to call into Deprecated.warning (module Deprecated.jsm ) whenever we call FileUtils.getDir().

Now, the more complicated part is to make this happen only during startup/shutdown. For this, you will need to implement a xpcom component in JavaScript, to determine the current stage of execution you're in:
- before sessionstore-windows-restored (in this case, the call is deprecated);
- after sessionstore-window-restored and before quit-application-granted (in this case, the call is ok);
- after quit-application-granted (in this case, the call is deprecated again).

Take a look at https://developer.mozilla.org/en-US/docs/Observer_Notifications and https://developer.mozilla.org/en-US/docs/XPCOM/Receiving_startup_notifications for more details.

Comment 3

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

Actually, let's not handle the second part of the bug yet – bug 881667 will make it much simpler.

Comment 4

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

Are you still working on this, Abhishek?

Comment 5

[Abhishek Potnis [:avp]]

No Yoric, Sorry, I won't be able to work on this.

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

I would like to take this. Could you please give me more guide? Thank you.

Comment 7

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

Have you already looked at comment 2?

Comment 8

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

Let's keep this bug for when we have made OS.File startup fast enough.

Comment 9

[Marco Castelluccio [:marco]]

Let's keep this open (sometimes FileUtils.getDir(..., ..., true) is used even if it is unnecessary, so we don't always need OS.File).

Comment 10

[kenrick95]

Hi, this looks interesting and would like to try it out.

May I confirm if now the required things to do is to only add Deprecate.warning call in FileUtils.getDir when the 3rd parameter (shouldCreate) is passed as true? Do I need to handle calling Deprecate.warning only during startup/shutdown? If so, since bug 881667 has been closed, what's the recommended way to know whether it is called during startup/shutdown?

Thanks :)

Comment 11

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

That's great :)

May I confirm if now the required things to do is to only add Deprecate.warning call in FileUtils.getDir when the 3rd parameter (shouldCreate) is passed as true?

Exactly.

Do I need to handle calling Deprecate.warning only during startup/shutdown?

Indeed.

If so, since bug 881667 has been closed, what's the recommended way to know whether it is called during startup/shutdown?

Well, it has been closed because it has been fixed :)

These days, the simplest way to know whether we're during startup is to call nsIAppStartup.

Something like

let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);

if (appStartup.startingUp) {
  // We're starting up.
}

if (appStartup.shuttingDown) {
  // We're shutting down.
}

Comment 12

[kenrick95]

Got it!

I think I would be able to send a patch soon. But to make sure, how do you suggest to test this behavior?

Thanks

Comment 13

[kenrick95]

Attachment: Bug 921157 - Deprecate FileUtils.getDir(..., ..., true) during startup/shutdown

Because passing true to the 3rd parameter (shouldCreate)
causes main thread I/O and should be avoided
especially during startup/shutdown

Comment 14

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

Unfortunately, I don't see any good way to test this. Let's first test that it doesn't break anything :)

Comment 15

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

Florian, do you have any idea on how we could test this?

Comment 16

[Florian Quèze [:florian]]

Do you want a test to verify that the deprecation warning has been emitted, or a test to ensure this is not being used during startup/shutdown?

I think you could have Deprecated.jsm add profiler markers that should be possible to access during startup tests.

Comment 17

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

Do you want a test to verify that the deprecation warning has been emitted, or a test to ensure this is not being used during startup/shutdown?

The former for the time being.

I think you could have Deprecated.jsm add profiler markers that should be possible to access during startup tests.

Could you mentor kenrick95 on this?

Comment 18

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

Well, let's start with checking that it doesn't break anything:

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

Comment 19

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

It seems to work, so let's land it.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf1b45188567
Deprecate FileUtils.getDir(..., ..., true) during startup/shutdown

Comment 21

[Atila Butkovits]

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

Comment 22

[Worcester12345]

Should this bug get flagged with the "Perf" key word?