Closed Bug 1651008 Opened 3 months ago Closed 2 months ago

Menus API does not throw meaningful error for invalid URL patterns

Categories

(WebExtensions :: General, defect, P5)

defect

Tracking

(firefox81 fixed)

RESOLVED FIXED
81 Branch
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.

Assignee: nobody → deeps.karanji2
Mentor: rob
Severity: -- → S4
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P5

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?

Flags: needinfo?(rob)

(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).

Flags: needinfo?(rob)

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.

Flags: needinfo?(rob)

(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.

Flags: needinfo?(rob)

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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

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.

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.

Flags: needinfo?(cneiman)
Flags: needinfo?(cneiman)
You need to log in before you can comment on or make changes to this bug.