1738246 - Remove OS.File usage from various browser/components/sessionstore/ tests

Status: RESOLVED FIXED

(Firefox :: Session Restore, task)

Description

[Mark Banner (:standard8)]

Using OS.File for file access is now obsolete and can be replaced by code using IOUtils. Working towards this, in this bug we want remove all OS.File usage from these files:

• browser/components/sessionstore/test/head.js
• browser/components/sessionstore/test/unit/head.js
• browser/components/sessionstore/test/unit/test_backup_once.js
• browser/components/sessionstore/test/unit/test_histogram_corrupt_files.js
• browser/components/sessionstore/test/unit/test_migration_lz4compression.js

The code in question can be found here.

There is background information here about how to migrate from OS.File to IOUtils. If you have questions, please ask.

To run the tests after you've built, you can run ./mach mochitest path/to/file. You should also check ESLint before commiting: ./mach eslint browser/.

I'm happy to mentor this. Note this bug will be auto-assigned when the first patch is attached, but feel free to comment that you're working on it.

Comment 1

[onuohaoluebube05]

Attachment: Bug 1738246 - Removed OS.File usage from various browser/components/sessionstore/ tests r?Standard8

Comment 2

[onuohaoluebube05]

Hello,

When i run the test ./mach mochitest browser/components/sessionstore/test/i got this

        Passed: 1597
        Failed: 1
        Todo: 9
        Mode: e10s
*** End BrowserChrome Test Results ***
21:26.51 INFO Buffered messages finished
21:26.53 SUITE_END
21:26.77
Overall Summary
===============

mochitest-browser
~~~~~~~~~~~~~~~~~
Ran 1783 checks (1607 subtests, 176 tests)
Expected results: 1766
Skipped: 16 tests
Unexpected results: 1
  subtest: 1 (1 fail)

Unexpected Results
------------------
browser/components/sessionstore/test/browser_restore_container_tabs_oa.js
  FAIL A promise chain failed to handle a rejection: can't access property "getAttribute", tab is undefined - stack: crashFrame@resource://testing-common/BrowserTestUtils.jsm:1956:11
Rejection date: Fri Oct 29 2021 18:11:02 GMT+0100 (West Africa Standard Time) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 271
Stack trace:
resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:271
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1122```

Comment 3

[onuohaoluebube05]

I can't really figure out what i am doing wrong.

Please help me.

Thanks

Comment 4

[Mark Banner (:standard8)]

I suspect that failure is probably a random intermittent - I can't reproduce it locally with or without your patch, and I don't see that it would be related, so I think it is safe to ignore.

I also just realised that you need to run these tests as well:

./mach xpcshell-test browser/components/sessionstore/test/unit/

These are failing at the moment.

Comment 5

[onuohaoluebube05]

Hello.

I think to successfully remove this var File = OS.File i should change all the occurence of File to IOUtilis in browser/components/sessionstore/test/unit/test_backup_once.js ?

Comment 6

[Mark Banner (:standard8)]

(In reply to onuohaoluebube05 from comment #5)

I think to successfully remove this var File = OS.File i should change all the occurence of File to IOUtilis in browser/components/sessionstore/test/unit/test_backup_once.js ?

Yes, that's correct - File was previously acting as just an alias.

Comment 7

[onuohaoluebube05]

Hello,

I ran ./mach xpcshell-test browser/components/sessionstore/test/unit/ and was able to fix the errors except browser/components/sessionstore/test/unit/test_histogram_corrupt_files.js when i tried debugging i realised that when switching from let s = await IOUtils.read(backups[key]); to let s = await OS.File.read(backups[key]); in the promise_reset_session function the test fail because backups[key] returns a relative path and while OS.File.read can accept a relative path, IOUtils.read accepts only absolute paths

How do i go about this?, do I need to build the absolute path manually? if so how ?

Thanks

Comment 8

[Mark Banner (:standard8)]

It looks like the path is relative to the test directory, so I think you can do this to get the full path:

PathUtils.join(do_get_cwd().path, backups[key])

Comment 9

[onuohaoluebube05]

(In reply to Mark Banner (:standard8) from comment #8)

It looks like the path is relative to the test directory, so I think you can do this to get the full path:

PathUtils.join(do_get_cwd().path, backups[key])

Okay

Thank you

Comment 10

[onuohaoluebube05]

Hello

I used PathUtils.join(do_get_cwd().path, backups[key]) and got dis error NS_ERROR_FILE_UNRECOGNIZED_PATH. I was eventually able to get it to stop throwing the error with PathUtils.joinRelative()

but the test still fail with the error below

FAIL browser/components/sessionstore/test/unit/test_histogram_corrupt_files.js - xpcshell return code: 0
FAIL test_all_files_corrupt - [test_all_files_corrupt : 126] One probe for the 'true' bucket. - 0 == 1

I don't know what could be the problem.
I will include a snippet of the entire function I have below

Thanks

Comment 11

[onuohaoluebube05]

function promise_reset_session(backups = {}) {
  return (async function() {
    // Reset the histogram.
    Telemetry.getHistogramById(HistogramId).clear();

    // Reset the contents of the backups directory
    IOUtils.makeDirectory(SessionFile.Paths.backups);
    for (let key of SessionFile.Paths.loadOrder) {
      if (backups.hasOwnProperty(key)) {
        let basePath = do_get_cwd().path;
        let relativePath = backups[key]; 
        relativePath = relativePath.replace("/", "\\")
        const fullPath = PathUtils.joinRelative(basePath, relativePath)      
        let s = await IOUtils.read(fullPath);
        await IOUtils.write(SessionFile.Paths[key], s, {
          compress: true,
        });
      }
      await IOUtils.remove(SessionFile.Paths[key]);
    }
  })();
}

Comment 12

[Mark Banner (:standard8)]

Please can you post the latest version of the patch phabricator, it'll be easier for me to try it out and take a look.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8dd20735b40c
Removed OS.File usage from various browser/components/sessionstore/ tests r=Standard8

Comment 14

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/8dd20735b40c

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

I think something must have gone wrong in this patch as tonight's Nightly on update lost my previous session. Another user has also noted he lost his session after the update today.

Tonight Buildconfig cset: https://hg.mozilla.org/mozilla-central/rev/d2b5e7cc2dbbd2f59082be5fb832b71fcdd46407

Comment 16

[Greg ( Hank1 )]

my sessionrestore is also gone. now how is one supposed to get back to there session if there's a power blackout or something like that?

Comment 17

[Mathew Hodson]

This patch only modified tests, so it's unlikely to have caused the issue you are seeing. You should file a new bug.

Comment 18

[Adrian Sampaleanu]

I've also lost my session with the latest build.

Comment 19

[Marco Bonardo [:mak]]

It was not this fix. Please go to Bug 1740987.