1854489 - Unhandled catched exceptions should be re-thrown.

Status: UNCONFIRMED

(Firefox :: Session Restore, task)

Description

[Daniel Hofmann]

Steps to reproduce:

1. Run Firefox on a 64-bit architecture with at least 4GB of RAM.
2. Create a valid-looking "/sessionstore-backups/recovery.jsonlz4" file which decompresses to at least 1073741824 bytes, or have a "/sessionstore-backups/recovery.json" file which has a size of at least 1073741824 bytes.
3. Try to load that session (e.g. by using the session recovery mechanism).

Actual results:

Observe that loading this session fails, without any error message.

Expected results:

Loading this session should fail, but with an error message. (Alternatively, it would be even better if loading the session would also succeed, but this bug report is not about that.)

======
Analysis:

https://hg.mozilla.org/mozilla-central/file/6aaf2cf582814f5a76c09fa2ac7cc760a5886134/browser/components/sessionstore/SessionFile.sys.mjs#l269 contains this code:

  } catch (ex) {
    if (DOMException.isInstance(ex) && ex.name == "NotFoundError") {
      exists = false;
    } else if (
      DOMException.isInstance(ex) &&
      ex.name == "NotAllowedError"
    ) {
      // The file might be inaccessible due to wrong permissions
      // or similar failures. We'll just count it as "corrupted".
      console.error("Could not read session file ", ex);
      corrupted = true;
    } else if (ex instanceof SyntaxError) {
      console.error(
        "Corrupt session file (invalid JSON found) ",
        ex,
        ex.stack
      );
      // File is corrupted, try next file
      corrupted = true;
    }
  } finally {

This code is faulty in the following subtle way: If exception "ex" is any other kind of exception than the ones this code tests for, then this exception is swallowed (not further propagated to the callers). Swallowing exceptions is wrong. Instead, the exception should be re-thrown. If re-thrown, the exception would propagate to the callers of this code, giving them a change to notify the user that something went wrong and giving the user a hint what could it be that went wrong.

(The actual exception in this example is an InternalError with the message "allocation size overflow". But the problem is generic, it would also happen in case of any other unhandled exception.)

The following code would be more correct:

  } catch (ex) {
    if (DOMException.isInstance(ex) && ex.name == "NotFoundError") {
      exists = false;
    } else if (
      DOMException.isInstance(ex) &&
      ex.name == "NotAllowedError"
    ) {
      // The file might be inaccessible due to wrong permissions
      // or similar failures. We'll just count it as "corrupted".
      console.error("Could not read session file ", ex);
      corrupted = true;
    } else if (ex instanceof SyntaxError) {
      console.error(
        "Corrupt session file (invalid JSON found) ",
        ex,
        ex.stack
      );
      // File is corrupted, try next file
      corrupted = true;
    } else {
      throw ex;
    }
  } finally {

Note that swallowing the exception was not intentional. It was introduced by this changeset: https://hg.mozilla.org/mozilla-central/rev/eb4b44b2eb026ce5a4c126a8f6a44af95ca3c7ed#l4.12

It is clear from this changeset that the old version actually did not swallow the exception, and the new version did. But it also clear from the commit message "Enable eslint of browser/components/sessionstore/." that no semantic changes were intended, only syntactic changes. However, there is a subtle semantic change, namely from propagating exceptions which were not explicitly handled to not propagating exceptions which were not explicitly handled.

Therefore, this subtle semantic change needs to be undone.

Comment 1

[Ina Popescu, Desktop QA]

Thank you for submitting this report. Setting the component to have the developer involved.
If this is not the correct component, please feel free to change it to a more appropriate one.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:sclements, could you have a look please?

For more information, please visit BugBot documentation.