873026 - Cancelling the safe mode dialog doesn't work

Status: RESOLVED WORKSFORME

(Firefox :: General, defect)

Description

[Mike Kaply [:mkaply]]

When you press cancel (Escape or close) in the safe mode dialog, it's supposed to simply close Firefox:

function onCancel() {
  appStartup.quit(appStartup.eForceQuit);
}

Unfortunately:

const appStartup = Services.startup;

and Services.jsm is never included.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Add import for Services.jsm

I thought about replacing all appStartup instances with Services.startup, but I think this was done for readability.

So this is the simplest fix.

Comment 2

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

"Doesn't work" as in doesn't actually quit Firefox? Seems to work for me (Mac, both on trunk and in 21) - is explicitly quitting here actually necessary?

Comment 3

[Mike Kaply [:mkaply]]

> "Doesn't work" as in doesn't actually quit Firefox? Seems to work for me (Mac, both on trunk and in 21) - is explicitly quitting here actually necessary?

Yes, doesn't quit Firefox. Pressing escape starts the browser in safe mode, and when I look at the console, I see "Services not defined".

And yes, explicitly quitting here is necessary. There needs to be an option for a user if they didn't mean to start safe mode.

Comment 4

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

That's not what I'm seeing, at least on Mac, so I'd like to understand that difference.

Comment 5

[Mike Kaply [:mkaply]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> That's not what I'm seeing, at least on Mac, so I'd like to understand that
> difference.

Closing a dialog on Mac doesn't trigger cancel processing, because it doesn't use OK/Cancel. IT uses instantApply.
When you close the dialog on Mac, none of the other processing in the dialog happens at all, so the app is never started.

That might be what happened here - this code was only tested on Mac. Either way, it's pretty clear that the code is referencing Services without including Services.jsm.

Comment 6

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

(In reply to Michael Kaply (mkaply) from comment #5)
> Closing a dialog on Mac doesn't trigger cancel processing, because it
> doesn't use OK/Cancel. IT uses instantApply.
> When you close the dialog on Mac, none of the other processing in the dialog
> happens at all, so the app is never started.

I think you're confusing dialogs and prefpanes. This is just a normal dialog, not a pref pane, so there shouldn't be any Mac/other differences AFAIK.

> That might be what happened here - this code was only tested on Mac. Either
> way, it's pretty clear that the code is referencing Services without
> including Services.jsm.

Yes, obviously the code is wrong - I just want to know whether the best fix is to remove the broken code, rather than fixing it.

Comment 7

[Mike Kaply [:mkaply]]

This is definitely a Windows only problem, and I have no explanation.

I verified that in a normal dialog, closing the dialog definitely goes down the cancel case.

I've also verified that Mac fails in the cancel case because Services isn't defined.

We probably need to bring a Mac person into the discussion to figure out what is going on.

Comment 8

[Justin Dolske [:Dolske]]

Hmm, seems like it _should_ work -- safeMode.js is loaded via safeMode.xul... Which will have already loaded resetProfile.js, which _does_ import Services.jsm. The patch here (to have safeMode.js also import Services.jsm) is good practice -- relying on a semi-related script loading first is fragile -- but it shouldn't actually fix anything.

If I add some logging to safeMode.js as:

 function onCancel() {
+dump("----- onCancel -----\n");
+dump("appStartup is: " + appStartup + "\n");
   appStartup.quit(appStartup.eForceQuit);
 }

and then start my OS X build in safe-mode and press Esc, I get:

----- onCancel -----
appStartup is: [xpconnect wrapped nsIAppStartup @ 0x10d02b120 (native @ 0x10875e4c0)]

So, this should and is working for me. Do you have anything else in your build that's unusual? (Let me try updating my build -- I'm on a May 6th m-c pull).

Comment 9

[Justin Dolske [:Dolske]]

Still works fine with a clobber build of 8ca260fe91e3.

Comment 10

[Mike Kaply [:mkaply]]

I'm quite confused now. I can't recreate either.  Argh. I'll try to figure out what is going on here. Don't bother with anything else.

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 750444 [details] [diff] [review]
Add import for Services.jsm

Still a problem for you or did it get sorted out?

Comment 12

[Mike Kaply [:mkaply]]

Still researching. I think I saw a side effect of something else that I was doing.

I think there's a bug here, I just can't narrow down exactly what I did to cause it.