618721 - If page-mod's 'include' option's value is invalid, incorrect error is shown.

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101211 Firefox/4.0b8pre
Build Identifier: 

I was trying to play around with SDK 1.0b1, and wanted to use the page-mod module.

I wanted the page mod to run on all pages in the roosterteeth.com domain, so I wrote my code like this:

    var pageMod = require("page-mod");
     
    pageMod.PageMod({
      include: ["roosterteeth.com"],
      contentScriptWhen: 'ready',
      contentScript: 'document.body.innerHTML = ' +
                     ' "<h1>Page matches ruleset</h1>";'
    }); 


Now when I tried to do cfx run, I get the following error:
Error: The PageMod must have a string or array `include` option.


This threw me off, because there clearly IS an include option in there. I tried removing the array brackets from it, and it still gave me that error.

Now, when I replaced "roosterteeth.com" with "*", it ran just fine.

Looking through the documentation, I should have used "*.roosterteeth.com", to correctly use match-pattern's... pattern.

It'd be nice if the SDK told me that I failed to use match-pattern correctly, instead of being told that 'include' isn't there.

Reproducible: Always

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Oh, I see what's going on.

match-pattern is throwing an error on line 61, but page-mod's try-catch block on lines 112-120 are eating it and replacing it with the "must have a string or array" error.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Let match-pattern's error bubble up

This lets match-pattern's more accurate error bubble up.

What was the try/catch's original error supposed to prevent? Other than string and array, what could it be (numeric, object)? It'd probably be better to explicitly check those kinds of things than just a catch-all like it was.

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: v2

This lets match-pattern's errors bubble up AND it validates 'include' against numbers and non-Array objects.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 512582 [details] [diff] [review]
v2

>diff --git a/packages/addon-kit/lib/page-mod.js b/packages/addon-kit/lib/page-mod.js
>+    // Validate 'include'
>+    if(typeof(include) == "object" && !Array.isArray(include))
>+      throw new Error(ERR_INCLUDE)
>+    if(typeof(include) == "number")
>+      throw new Error(ERR_INCLUDE)

This should throw ERR_INCLUDE if include is undefined as well, since ERR_INCLUDE is more descriptive than the "pattern is undefined" exception that match-pattern will throw in that case, and to fix the test-page-mod.testPageModErrorHandling test, which expects ERR_INCLUDE.

Also, style nits: add a space between "if" and "(" and semi-colons at the ends of statements.

r=myk with those changes!

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Pushed with the changes requested in the review comments to get it in before today's 1.0b3 freeze:

https://github.com/mozilla/addon-sdk/compare/36c708f...30f4b49

I also removed the try/catch block, which is unnecessary now that we propagate the match-pattern exception unaltered.

Thanks for the fix!