Closed Bug 1307012 Opened 3 years ago Closed 3 years ago

WebExtension APIs should raise a meaningful error when the window ID is invalid

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(firefox53 verified)

VERIFIED FIXED
mozilla53
Tracking Status
firefox53 --- verified

People

(Reporter: robwu, Assigned: andy+bugzilla, Mentored)

Details

(Whiteboard: [good first bug]triaged)

Attachments

(1 file)

Similarly to https://hg.mozilla.org/mozilla-central/rev/57c5749e5b19612638a3ab08027431e6346c538e (from bug 1225215), but then for window IDs.

Test case:

browser.windows.get(123456789).catch(e => console.log(e.message));
// or chrome.windows.get(123456789, () => console.log(chrome.runtime.lastError.message));

Expected: Some meaningful error. E.g. Chrome has "No window with id: 123456789."
Actual: An unexpected error occurred


This might be a good first bug.
Mentor: bob.silverberg
Priority: -- → P3
Whiteboard: [good first bug]triaged
I am starting out. I would like to handle this if you can give some context and tips to start
(In reply to vgkrish619 from comment #1)
> I am starting out. I would like to handle this if you can give some context
> and tips to start

Bob, can you help here please?
Flags: needinfo?(bob.silverberg)
The file you want is:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-windows.js#58

You'll see that on line 58 we try and get the windowId. But we don't check its return value, if WindowManager.getWindow fails, it will return null. So you probably want to add in something to check if the value is null and raise an error.

I've got a patch pending that does something similar here (look for the "Invalid message"):

https://reviewboard.mozilla.org/r/85422/diff/2#index_header

You'll need to write a test that this works too. Tests are in the test directory, probably adding a test to:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_windows.js

Would work.

This assumes that you are familiar with building and testing Firefox. If not please go check out:

https://wiki.mozilla.org/Add-ons/Contribute#Improve_add-ons_in_Firefox
http://areweeveryoneyet.org/onramp/desktop.html

Sorry to steal your thunder there Bob, its just that I looked at this just the other day...
No problem, thanks for jumping in, Andy.
Flags: needinfo?(bob.silverberg)
Does that information help, is there anything else we can help you with on this bug?
Flags: needinfo?(vgkrish619)
Apologies for the delay, I did take a look at this. I would like to know if there is a visual error thrown or log I can see when the WindowManager.getWindow is null? I tried adding browser.windows.get(123456789).catch(e => console.log(e.message)); in the `ext-windows.js` and was getting errors on this code itself (I will update with the exact error message). 

I will go through `ext-windows.js` again, may be I am missing something.
No need to apologise. 

Yeah that sounds right, once this patch is completed, then that code should work just fine. When you reject the promise because of the invalid ID then it should propogate up to the catch. I just did a quick test using similar code for tabs:

browser.tabs.get(123456789).catch(e => console.log(e.message));

And I get this to the console.

Invalid tab ID: 123456789
Sorry folks, I am not getting around to working on this after this much promises I kept making. I would rater open this to anyone who would like to. I am removing myself from working on this.
Flags: needinfo?(vgkrish619) → needinfo?
No worries, thank you for wanting to help out in the first place.
Flags: needinfo?
Assignee: nobody → amckay
Comment on attachment 8815081 [details]
bug 1307012 raise an error on invalid id

https://reviewboard.mozilla.org/r/96068/#review96798

::: browser/components/extensions/test/browser/browser_ext_windows.js:37
(Diff revision 1)
>    yield BrowserTestUtils.closeWindow(raisedWin);
>  });
> +
> +add_task(function* testInvalidWindowId() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background: async function() {

Nit: `async background() {`
Attachment #8815081 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/628708af83de
raise an error on invalid id r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/628708af83de
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
I was able to reproduce this issue on Firefox 52.0a2 (2016-12-11) under Window 10 64-bit.

Verified as fixed on Firefox 53.0a1 (2016-12-11) under Windows 10 64-bit and Ubuntu 16.04 32-bit. The following error message is successfully displayed: “Invalid window ID: 123456789”.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.