Menus API does not throw meaningful error for invalid URL patterns
Categories
(WebExtensions :: General, defect, P5)
Tracking
(firefox81 fixed)
Tracking | Status | |
---|---|---|
firefox81 | --- | fixed |
People
(Reporter: deeps.karanji2, Assigned: deeps.karanji2, Mentored)
Details
Attachments
(1 file)
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0
Steps to reproduce:
When an invalid URL ("http://does/not/match") is passed, Menus API does not give an error message containing the invalid URL.
File: https://searchfox.org/mozilla-central/rev/9ad88f80aeedcd3cd7d7f63be07f577861727054/browser/components/extensions/parent/ext-menus.js#762,771
Actual results:
This is along the same lines as Bug #1637431 (https://bugzilla.mozilla.org/show_bug.cgi?id=1637431)
Expected results:
Further Information at:
https://phabricator.services.mozilla.com/D78466?id=292348#inline-453137
Mentor- Rob Wu
I believe the test for this must be added here https://searchfox.org/mozilla-central/rev/91d82d7cbf05a71954dfa49d0e43824c7c973e62/browser/components/extensions/test/browser/browser_ext_menus.js#289-294
.
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Thanks for assigning me :)
Quick Question:
https://searchfox.org/mozilla-central/rev/91d82d7cbf05a71954dfa49d0e43824c7c973e62/browser/components/extensions/test/browser/browser_ext_menus.js#289-294
Here, menu is created with invalid URL ["http://does/not/match"] has title set to "invisible", but https://searchfox.org/mozilla-central/rev/91d82d7cbf05a71954dfa49d0e43824c7c973e62/browser/components/extensions/test/browser/browser_ext_menus.js#295-300 has title "gamma", and https://searchfox.org/mozilla-central/rev/91d82d7cbf05a71954dfa49d0e43824c7c973e62/browser/components/extensions/test/browser/browser_ext_menus.js#329-330 expect the title to be "gamma", else it says dummy page.
Since I am trying to generate a message for menu created with invalid URL ["http://does/not/match"], may I please know why it is set to invisible and why its considered as a dummy page that shouldn't occur there?
Comment 2•4 years ago
|
||
(In reply to Deepika Karanji from comment #1)
Since I am trying to generate a message for menu created with invalid URL ["http://does/not/match"],
may I please know why it is set to invisible
The tab is http://example.com/
(line 308-311,318).
The "invisible" menu item for the "tab" context has documentUrlPatterns: ["http://does/not/match"]
, which does not match the tab's URL. Note that http://does/not/match
is a valid match pattern (and also a semantically valid URL). If you are looking for ways to test your patch, create an invalid URL pattern such as what you did in your previous patch for bug 1637431.
The "gamma" menu item for the "tab" context has documentUrlPatterns: ["http://example.com/"]
, which does match the tab's URL.
and why its considered as a dummy page that shouldn't occur there?
The test was introduced in bug 1359704, as a regression test to verify that the menu item for context "page" doesn't appear in menus from the "tab" context.
Side note: the isnot(gamma.label, "dummy", ...)
line is redundant because just two lines later there is the stronger check is(gamma.label, "gamma", ...)
(which would already fail if the "dummy" label were to be used).
Assignee | ||
Comment 3•4 years ago
|
||
I modified https://searchfox.org/mozilla-central/rev/9ad88f80aeedcd3cd7d7f63be07f577861727054/browser/components/extensions/parent/ext-menus.js#762,771 as follows:
if (createProperties.documentUrlPatterns != null) {
console.log("--------------MY FIX HERE--------------")
try{
this.documentUrlMatchPattern = parseMatchPatterns(
this.documentUrlPatterns,
{
restrictSchemes: this.extension.restrictSchemes,
}
);
} catch(e){
console.log("--------------", e);
throw(e);
}
console.log("----------PARSED------------")
console.log("~~~~~~~~~~", this.documentUrlMatchPattern)
}
I wrote this as the test:
add_task(async function test_invalid_menu_url() {
const extension = ExtensionTestUtils.loadExtension({
manifest: {
permissions: ["menus"],
},
async background() {
try{
browser.menus.create({
title: "invalid_url",
contexts: ["tab"],
documentUrlPatterns: ["test1"],
});
} catch(e){
console.log("-----TEST_ERROR----", e);
is(e,
"ExtensionError: Invalid url pattern: test1",
"invalid menu url pattern")
}
},
});
await extension.startup();
await extension.awaitFinish("test_invalid_menu_url");
await extension.unload();
});
But this is my output:
Entering test bound test_invalid_menu_url
0:14.91 INFO Extension loaded
0:14.95 GECKO(17002) console.log: WebExtensions: --------------MY FIX HERE--------------
0:14.95 GECKO(17002) console.log: WebExtensions: -------------- Message: ExtensionError: Invalid url pattern: test1
0:14.95 INFO Console message: [JavaScript Error: "Unchecked lastError value: Error: Invalid url pattern: test1" {file: "resource://gre/modules/ExtensionCommon.jsm" line: 765}]
withLastError@resource://gre/modules/ExtensionCommon.jsm:765:12
getAPI/create/<@chrome://browser/content/child/ext-menus.js:148:23
promise callback*create@chrome://browser/content/child/ext-menus.js:147:14
callFunction@resource://gre/modules/ExtensionCommon.jsm:1063:37
callFunction/<@resource://gre/modules/ExtensionChild.jsm:680:40
callAndLog@resource://gre/modules/ExtensionChild.jsm:665:14
callFunction@resource://gre/modules/ExtensionChild.jsm:680:17
stub@resource://gre/modules/Schemas.jsm:2702:30
background@moz-extension://8b9972d3-df6f-4408-9ee5-ba45d998c453/%7B916bfdd3-6d25-47ce-be0d-f8c094710311%7D.js:3:25
@moz-extension://8b9972d3-df6f-4408-9ee5-ba45d998c453/%7B916bfdd3-6d25-47ce-be0d-f8c094710311%7D.js:25:7
And then the test times out.
I explored ExtensionCommon.jsm:765:12
, but can I get some help in understanding what unchecked error value
means?
From what I understood, it means an extension is sending a message that doesn't have a corresponding listener, and not handling the error correctly, but I'm not exactly sure how to fix that.
Comment 4•4 years ago
|
||
(In reply to Deepika Karanji from comment #3)
I explored
ExtensionCommon.jsm:765:12
,
I suggest to not only look at the top of the stack, but also higher up the stack. Functions in shared/common code such as ExtensionCommon.jsm are used by others, and the rest of the stack trace will often provide the context needed to understand the issue. In this case, the relevant code is in child/ext-menus.js - see below for the explanation.
but can I get some help in understanding what
unchecked error value
means?
From what I understood, it means an extension is sending a message that doesn't have a corresponding listener, and not handling the error correctly, but I'm not exactly sure how to fix that.
An unchecked error is when a callback is called with browser.runtime.lastError
set, but the error is not checked in the callback. browser.menus.create
is the only async extension API that doesn't return a promise (bug 1527979). If you want a normal promise, you will need to wrap the call in a promise and add a callback that calls resolve
or reject
depending on whether browser.runtime.lastError
is set.
Here are two examples, one that expects no error, and one that expects an error: https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/browser/components/extensions/test/browser/browser_ext_menus_errors.js#30-53
Every other async extension API in Firefox supports Promises, so you will not encounter this issue elsewhere (unless you pass a callback, which we also support because the original API from Chrome supported callbacks).
PS. Thanks for showing what you did (the code snippet), the error message and your interpretation of it. This is a good way to communicate where you are at, and what you need help with.
Assignee | ||
Comment 5•4 years ago
|
||
Assignee | ||
Comment 6•4 years ago
|
||
ReviewBot gives the following error for the attachment:
The analysis task source-test-doc-upload failed, but we could not detect any issue.
Please check this task manually.
If you see a problem in this automated review, please report it here.
I discussed this with the folks on Introduction channel and they said its a network error bug and can be ignored.
Pushed by abutkovits@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b2901a7b9158 Gives meaningful menus create error message. r=robwu
Comment 8•4 years ago
|
||
bugherder |
Comment 9•4 years ago
|
||
Thanks so much for the patch, Deepika! 🎉 Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition
Would you be interested in creating a profile on mozillians.org? I'd be happy to vouch for you.
Assignee | ||
Comment 10•4 years ago
|
||
Hi Caitlin!
Thank you!
Yes, I'd love to create a profile on mozillians.org. I have sent you an invite on the matrix app to discuss it further.
Updated•4 years ago
|
Description
•