1852509 - TabManager.getTabBrowser should throw an explicit error if window is invalid

Status: NEW

(Remote Protocol :: Agent, task, P3)

Description

[Julian Descottes [:jdescottes]]

At the moment, this API will throw if win is null or falsy when trying to access win.gBrowser.

We could make it throw a meaningful exception and review the call sites to make sure we either guard or handle the error.

Comment 1

[Julian Descottes [:jdescottes]]

The goal of this bug is to add a check in the following method:

  getTabBrowser(win) {
    if (lazy.AppInfo.isAndroid) {
      return new lazy.MobileTabBrowser(win);
    } else if (lazy.AppInfo.isFirefox) {
      return win.gBrowser;
    }

    return null;
  },

and throw an explicit error if win is null/falsy.

A similar error was added for invalid browsing contexts to another method: https://searchfox.org/mozilla-central/rev/699a7704521354cac1e6a0679029c89ca899e25c/remote/shared/TabManager.sys.mjs#263-267

Considering that the current method is already throwing when providing an invalid window, I don't think we have to update the call sites for getTabBrowser.

We should add tests to check that an error is thrown as expected, for instance in remote/shared/test/xpcshell/test_TabManager.js

Comment 2

[peterodejobi9]

Hi please can i be assigned this bug ? And where is this bug file located and how can i replicate this issue on my machine ?

Comment 3

[Julian Descottes [:jdescottes]]

Hi there! Thanks for your interest, I'll try to help you get started.

(In reply to peterodejobi9 from comment #2)

Hi please can i be assigned this bug ? And where is this bug file located and how can i replicate this issue on my machine ?

The bug will be automatically assigned to you when you submit a patch for it on phabricator.

In the previous comment, you have a link to searchfox.org pointing to the file that should be updated. If you have a mozilla-central checkout, the file is called TabManager.sys.mjs should be at <your_repository_root>/remote/shared/TabManager.sys.mjs

The bug itself is not really easy to "reproduce", it's about changing how the getTabBrowser method behaves when used with incorrect arguments (which can happen during runtime, but under very random conditions). So the best here will be to add unit tests in remote/shared/test/xpcshell/test_TabManager.js which you can run with ./mach test remote/shared/test/xpcshell/test_TabManager.js.

Don't hesitate to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you have more questions about this specific bug (there are also rooms to help you get started with the overall project)

Comment 4

[peterodejobi9]

Hi thank you for your response. I am an outreachy applicant i downloaded the firefox source code i don't know if that's the right code base for this file, because i can't find the folder or file you're talking about

Comment 5

[peterodejobi9]

(In reply to peterodejobi9 from comment #4)

Hi thank you for your response. I am an outreachy applicant i downloaded the firefox source code i don't know if that's the right code base for this file, because i can't find the folder or file you're talking about

Never Mind i found it.

Comment 6

[peterodejobi9]

(In reply to Julian Descottes [:jdescottes] from comment #3)

Hi there! Thanks for your interest, I'll try to help you get started.

(In reply to peterodejobi9 from comment #2)

Hi please can i be assigned this bug ? And where is this bug file located and how can i replicate this issue on my machine ?

The bug will be automatically assigned to you when you submit a patch for it on phabricator.

In the previous comment, you have a link to searchfox.org pointing to the file that should be updated. If you have a mozilla-central checkout, the file is called TabManager.sys.mjs should be at <your_repository_root>/remote/shared/TabManager.sys.mjs

The bug itself is not really easy to "reproduce", it's about changing how the getTabBrowser method behaves when used with incorrect arguments (which can happen during runtime, but under very random conditions). So the best here will be to add unit tests in remote/shared/test/xpcshell/test_TabManager.js which you can run with ./mach test remote/shared/test/xpcshell/test_TabManager.js.

Don't hesitate to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you have more questions about this specific bug (there are also rooms to help you get started with the overall project)

Hi so when i am doing a unit test for the getTabBrowser does it need to be asynchronous ?

Comment 7

[peterodejobi9]

(In reply to Julian Descottes [:jdescottes] from comment #3)

Hi there! Thanks for your interest, I'll try to help you get started.

(In reply to peterodejobi9 from comment #2)

Hi please can i be assigned this bug ? And where is this bug file located and how can i replicate this issue on my machine ?

The bug will be automatically assigned to you when you submit a patch for it on phabricator.

In the previous comment, you have a link to searchfox.org pointing to the file that should be updated. If you have a mozilla-central checkout, the file is called TabManager.sys.mjs should be at <your_repository_root>/remote/shared/TabManager.sys.mjs

The bug itself is not really easy to "reproduce", it's about changing how the getTabBrowser method behaves when used with incorrect arguments (which can happen during runtime, but under very random conditions). So the best here will be to add unit tests in remote/shared/test/xpcshell/test_TabManager.js which you can run with ./mach test remote/shared/test/xpcshell/test_TabManager.js.

Don't hesitate to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you have more questions about this specific bug (there are also rooms to help you get started with the overall project)

Hi so looking at the code and the report, it says that the API will throw when win is null/falsy but if said condition is true it just returns null it doesn't throw anything.

Comment 8

[peterodejobi9]

Attachment: WIP: Bug 1852509 - [browser tab] TabManager.getTabBrowser throws an explicit error when window is invalid.

Comment 9

[peterodejobi9]

(In reply to Julian Descottes [:jdescottes] from comment #3)

Hi there! Thanks for your interest, I'll try to help you get started.

(In reply to peterodejobi9 from comment #2)

Hi please can i be assigned this bug ? And where is this bug file located and how can i replicate this issue on my machine ?

The bug will be automatically assigned to you when you submit a patch for it on phabricator.

In the previous comment, you have a link to searchfox.org pointing to the file that should be updated. If you have a mozilla-central checkout, the file is called TabManager.sys.mjs should be at <your_repository_root>/remote/shared/TabManager.sys.mjs

The bug itself is not really easy to "reproduce", it's about changing how the getTabBrowser method behaves when used with incorrect arguments (which can happen during runtime, but under very random conditions). So the best here will be to add unit tests in remote/shared/test/xpcshell/test_TabManager.js which you can run with ./mach test remote/shared/test/xpcshell/test_TabManager.js.

Don't hesitate to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you have more questions about this specific bug (there are also rooms to help you get started with the overall project)

https://phabricator.services.mozilla.com/D190794 Here is my patch. please check it out.

Comment 10

[Julian Descottes [:jdescottes]]

(In reply to peterodejobi9 from comment #9)

https://phabricator.services.mozilla.com/D190794 Here is my patch. please check it out.

Thanks for the patch! I imagine you're trying to fix the issue with the unrelated files included in the diff, but the patch is in the right direction. Just left a couple of comments on phabricator worth checking before submitting another version.

Comment 11

[Julian Descottes [:jdescottes]]

Hi there,

Let us know if you have issues creating a patch with the appropriate files only. Feel free to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you need help.

Comment 12

[peterodejobi9]

(In reply to Julian Descottes [:jdescottes] from comment #11)

Hi there,

Let us know if you have issues creating a patch with the appropriate files only. Feel free to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you need help.

I will thank you!. One more thing when window is invalid does it have anything to do with the tab being closed or the Parent window being closed or the window crashing ?

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to peterodejobi9 from comment #12)

(In reply to Julian Descottes [:jdescottes] from comment #11)

Hi there,

Let us know if you have issues creating a patch with the appropriate files only. Feel free to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you need help.

I will thank you!. One more thing when window is invalid does it have anything to do with the tab being closed or the Parent window being closed or the window crashing ?

It can be for many reasons, since the method we are modifying is a helper which might be called in various situations.
However the most common occurrence is that a window/tab got closed.

In case you are asking this to update the error message: we can't say for certain why the passed window argument is invalid, so the message you had in your initial patch looked fine.

Comment 14

[peterodejobi9]

(In reply to Julian Descottes [:jdescottes] from comment #13)

(In reply to peterodejobi9 from comment #12)

(In reply to Julian Descottes [:jdescottes] from comment #11)

Hi there,

Let us know if you have issues creating a patch with the appropriate files only. Feel free to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you need help.

I will thank you!. One more thing when window is invalid does it have anything to do with the tab being closed or the Parent window being closed or the window crashing ?

It can be for many reasons, since the method we are modifying is a helper which might be called in various situations.
However the most common occurrence is that a window/tab got closed.

In case you are asking this to update the error message: we can't say for certain why the passed window argument is invalid, so the message you had in your initial patch looked fine.

Okay thank you! I'm working on the test file right now, in the right file this time!.

Comment 15

[peterodejobi9]

Attachment: WIP: Bug 1852509 - [browser tab] TabManager.getTabBrowser throws an explicit error when window is invalid.

Comment 16

[peterodejobi9]

(In reply to Julian Descottes [:jdescottes] from comment #13)

(In reply to peterodejobi9 from comment #12)

(In reply to Julian Descottes [:jdescottes] from comment #11)

Hi there,

Let us know if you have issues creating a patch with the appropriate files only. Feel free to join the chat at https://chat.mozilla.org/#/room/#webdriver:mozilla.org if you need help.

I will thank you!. One more thing when window is invalid does it have anything to do with the tab being closed or the Parent window being closed or the window crashing ?

It can be for many reasons, since the method we are modifying is a helper which might be called in various situations.
However the most common occurrence is that a window/tab got closed.

In case you are asking this to update the error message: we can't say for certain why the passed window argument is invalid, so the message you had in your initial patch looked fine.

Hi Julian, i am working on the testing file, You requested in the review that i test with other values and make the test fail but the issue is when i use Assert.throw and the error doesn't match, the function exist and the rest of the test aren't executed.

Do i use a separate add_task function for each test ?

Comment 17

[peterodejobi9]

Attachment: WIP: Bug 1852509 - [browser tab] TabManager.getTabBrowser throws an explicit error when window is invalid

Depends on D191797

Comment 18

[Julian Descottes [:jdescottes]]

(In reply to peterodejobi9 from comment #16)

Hi Julian, i am working on the testing file, You requested in the review that i test with other values and make the test fail but the issue is when i use Assert.throw and the error doesn't match, the function exist and the rest of the test aren't executed.

Do i use a separate add_task function for each test ?

Hi Peter, sorry for the delay. Did you check my review comments on https://phabricator.services.mozilla.com/D191797 ? That should answer your questions. Let us know if you need more guidance to finish the patch.

Comment 19

[Julian Descottes [:jdescottes]]

Peter, any update on your side? Let us know if you don't have time to work on this anymore, we can reset the status of the bug.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Peter seems to be gone. Freeing up the bug so that someone else could continue the work.