Closed Bug 1269859 Opened 5 years ago Closed 5 years ago

quit-application-granted seems to behave differently in FF 44 vs 45

Categories

(Firefox :: Session Restore, defect)

45 Branch
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1284687

People

(Reporter: timsateroy, Unassigned)

References

Details

Steps to reproduce:
===================

1. Create a fresh profile in FF 45
2. Open a few tabs
3. Send the quit-application-granted[1] observer notification topic with nsIObserverService[2] using the browser console:

Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService).notifyObservers(null, "quit-application-granted", null);



Actual results:
===============

Firefox quit. Tabs are restored successfully on manual relaunch, without a crash report (correct).



Expected results:
=================

Not quite sure, I'm getting mixed signals:

1. In essence, the 'restart'[3] function in Vimperator sometimes breaks in FF 45.

The browser exits before 'nsIAppStartup.eRestart' is successfully executed, and the browser simply quits instead of restarting.

Related issue[4].

2. In FF 44 it will not quit when aforementioned notification is issued, like in FF 45.
3. Other addons and developers seem to struggle with the same [5][6]

----

I may be misunderstanding the correct use of the API, but it seems to me that something in 'quit-application-granted' or related methods/objects were changed between 44 and 45. 

Is this a regression, bug or maybe intentional change?
How should it be used, fixing the 'restart' function in Vimperator?



[1] - https://developer.mozilla.org/en-US/docs/Observer_Notifications#Application_shutdown
[2] - https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService#notifyObservers()
[3] - https://github.com/vimperator/vimperator-labs/blob/68ae0c183b5c09278c0d12c096f6033f0ea7f9e8/common/content/liberator.js#L992
[4] - https://github.com/vimperator/vimperator-labs/issues/460
[5] - https://github.com/mooz/keysnail/commit/cb37d9c04763306332f438987e26c48cd2bdf54a
[6] - http://stackoverflow.com/questions/36878005/javascript-firefox-restart
Flags: needinfo?(mconley)
The only thing I can think of is that the quit-application-granted handling for SessionStore in bug 1177310 affected this.

In that patch, on quit-application-granted, SessionStore does the work of flushing any remaining session state out of each window, and also closes each window.

Note that we're still able to run script after the signal is sent. Here's an example scratchpad:
```JavaScript

function restartIn5Seconds() {
  setTimeout(() => {
    Services.obs.notifyObservers(null, "quit-application-granted", null);
    Services.startup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
  }, 5000)
}

restartIn5Seconds();

```

This appears to restart the session for me.

So one thing that comes to mind is that perhaps the Vimperator script that is sending the quit-application-granted notification is trying to do something after that is throwing or returning early before it can get to the eRestart bit.

For example, this code here:

https://github.com/vimperator/vimperator-labs/blob/68ae0c183b5c09278c0d12c096f6033f0ea7f9e8/common/content/liberator.js#L1009

If this is removed, does the eRestart line get hit?
Flags: needinfo?(mconley) → needinfo?(timsateroy)
Thank you for taking a look into this, mconley. 

My original intention was to follow up on your suggestion as soon as I got the opportunity, but time has fled away from me. Sorry about the delay, I'll try to have a look at it before or during the next weekend.
Gentle ping to timss - any update on this?
My apologies for the delayed response.

I've experimented further, but not sure if I'm much wiser.
Testing has been done on Firefox ESR 45.2.0 (Linux).

---

1. Your `restartIn5Seconds()` function in the browser console seems to work as it should.

2. Running the same function from Vimperator however, does not work. It immediately exits after 'quit-application-granted' is issued.

Vimperator's javascript interpreter uses `eval()` internally.

3. In other words, it might seem that if 'quit-application-granted' is being issued from an addon, it'll halt and stop any code after it from executing. 

4. Removing the 'return' in [1] doesn't change anything, because it never gets to that point of the code.

5. If the browser console is open, the Vimperator restart function works with no alterations, both from the browser console or Vimperator.

I have no idea about this one. Made for some frustrating debugging.

---

Not sure where to go from here. Simply removing 'quit-application-granted' works, and is what other developers have done [2]. 
Would this now be the correct way to restart the browser from an addon?

---

[1]: https://github.com/vimperator/vimperator-labs/blob/68ae0c183b5c09278c0d12c096f6033f0ea7f9e8/common/content/liberator.js#L1009
[2]: https://github.com/mooz/keysnail/commit/cb37d9c04763306332f438987e26c48cd2bdf54a
Flags: needinfo?(timsateroy) → needinfo?(mconley)
Bug 1284687 landed recently, which may have fixed this for 48 and up. Would you mind testing Beta, DevEdition or Nightly to see if that's the case?
Flags: needinfo?(mconley) → needinfo?(timsateroy)
Tested using the original unmodified `restart()` function in Vimperator, as well as your `restartIn5Seconds()` function.

47.0.1: broken
48.0b7: broken
50.0a1: working

e10s disabled for Vimperator compability.

Seems to be fixed, thanks!
But shouldn't 48.0b7 work as well?
Flags: needinfo?(timsateroy) → needinfo?(mconley)
(In reply to timss from comment #6)
> Tested using the original unmodified `restart()` function in Vimperator, as
> well as your `restartIn5Seconds()` function.
> 
> 47.0.1: broken
> 48.0b7: broken
> 50.0a1: working
> 
> e10s disabled for Vimperator compability.
> 
> Seems to be fixed, thanks!
> But shouldn't 48.0b7 work as well?

According to https://bugzilla.mozilla.org/show_bug.cgi?id=1284687#c22, it was fixed in 48.0b9. Is your beta fully up to date?
Flags: needinfo?(mconley) → needinfo?(timsateroy)
Sorry, didn't see that b9 was specified. I downloaded from the channel website[1] and assumed this was up to date. 
Updated the beta release using the 'about' dialog I got version 48.0, and everything seems fine now.

Thanks for looking into this. I'm assuming it won't, but would the ESR channel (45) possibly have a release with this patch included? 

If there's nothing else, I'll mark this bug as resolved.

---

[1]: https://www.mozilla.org/en-US/firefox/channel/
Flags: needinfo?(timsateroy) → needinfo?(mconley)
See Also: → 1284687
(In reply to timss from comment #8)
> Thanks for looking into this. I'm assuming it won't, but would the ESR
> channel (45) possibly have a release with this patch included? 

I'm afraid ESR only gets security-critical updates, and the patch in question does not qualify.
Flags: needinfo?(mconley)
That's quite all right, but thought I'd ask anyway :-)

I don't think any of the 'resolved' statuses I can choose fits this bug. 
Care to pick one?
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1284687
You need to log in before you can comment on or make changes to this bug.