1195603 - Sync may prevent Firefox shutting down

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: sync-shutdown.txt

I started a "first sync" for a profile and attempted to shutdown Firefox during the Sync. It took a long time to shutdown and the log shows Sync bravely attempting to continue.

The log is from the first evidence of the shutdown request to the last entry written - there's around 60 from first to last (ie, it took over 60 seconds for Firefox to quit)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1195603-prevent-sync-from-blocking-app-shutdown..patch

Richard, this is a fairly simple patch that attempts to ignore "app shutdown" exceptions wherever they may be thrown (which is only when we do "spinningly" things. It's not attempting to reduce log noise etc, but instead to just stop most loops from bravely continuing in the face of shutdowns.

I would normally look for another reviewer, but I think you are probably one of the only people to understand the semantics of this stuff and it shouldn't take very long - let me know though if you are too busy and I'll find someone else.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8653923 [details] [diff] [review]
0001-Bug-1195603-prevent-sync-from-blocking-app-shutdown..patch

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

As I'm sure you know, this is incredibly fragile. But there's not much else we can do :/

::: services/common/async.js
@@ +135,5 @@
> +   * this will be used in exception handlers to allow such exceptions to
> +   * make their way to the top frame and allow the app to actually terminate.
> +   */
> +  isShutdownException(exception) {
> +    return exception && exception.result == Cr.NS_ERROR_ABORT && exception.message == "App. Quitting";

Can we add a custom flag to the exception that we raise so that our check can be simpler?

::: services/sync/modules/engines.js
@@ +1458,5 @@
>  
>            out.encrypt(this.service.collectionKeys.keyForCollection(this.name));
>            up.pushData(out);
>          }
> +        catch(ex if !Async.isShutdownException(ex)) {

Cuddle and paren-space while you're here.

@@ +1550,5 @@
>      try {
>        this._log.trace("Trying to decrypt a record from the server..");
>        test.get();
>      }
> +    catch(ex if !Async.isShutdownException(ex)) {

Cuddle and paren-space while you're here.

::: services/sync/modules/engines/history.js
@@ +221,5 @@
>            shouldApply = false;
>          } else {
>            shouldApply = this._recordToPlaceInfo(record);
>          }
> +      } catch(ex if !Async.isShutdownException(ex)) {

Space before paren.

::: services/sync/modules/record.js
@@ +235,5 @@
>        let record = new this._recordType(url);
>        record.deserialize(this.response);
>  
>        return this.set(url, record);
> +    } catch(ex if !Async.isShutdownException(ex)) {

Space

::: services/sync/modules/resource.js
@@ +402,5 @@
>      // The channel listener might get a failure code
>      try {
>        this._doRequest(action, data, callback);
>        return Async.waitForSyncCallback(cb);
> +    } catch(ex if !Async.isShutdownException(ex)) {

Space

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1a08315c0c25

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a08315c0c25