Closed Bug 1408066 Opened 6 years ago Closed 6 years ago

Addon.js install fails if path is incorrect

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
58 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: b4hand, Assigned: b4hand)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1408066 - Addon.js install fails if path is incorrect.
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
Log output:

platform darwin -- Python 3.6.0, pytest-3.2.2, py-1.4.34, pluggy-0.4.0 -- /Users/bforehand/Projects/mozilla/lockbox-extension/.tox/ui-tests/bin/python3.6
cachedir: .cache
driver: Firefox
sensitiveurl: .*
metadata: {'Python': '3.6.0', 'Platform': 'Darwin-17.0.0-x86_64-i386-64bit', 'Packages': {'pytest': '3.2.2', 'py': '1.4.34', 'pluggy': '0.4.0'}, 'Plugins': {'xdist': '1.18.2', 'variables': '1.6.1', 'selenium': '1.11.0', 'metadata': '1.5.0', 'instafail': '0.3.0', 'html': '1.16.0', 'base-url': '1.4.1'}, 'Base URL': '', 'Driver': 'Firefox', 'Capabilities': {}}
rootdir: /Users/bforehand/Projects/mozilla/lockbox-extension, inifile:
plugins: xdist-1.18.2, variables-1.6.1, selenium-1.11.0, metadata-1.5.0, instafail-0.3.0, html-1.16.0, base-url-1.4.1
collected 1 item

test/integration/test_extension.py::test_login ERROR

==================================================================================================================================== ERRORS ====================================================================================================================================
_________________________________________________________________________________________________________________________ ERROR at setup of test_login _________________________________________________________________________________________________________________________

selenium = <selenium.webdriver.firefox.webdriver.WebDriver (session="fde510c0-3210-db4e-9765-aee1491bada0")>

    @pytest.fixture(autouse=True)
    def selenium(selenium):
>       selenium.install_addon('my_addon.xpi')

test/integration/conftest.py:15:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.tox/ui-tests/lib/python3.6/site-packages/selenium/webdriver/firefox/webdriver.py:246: in install_addon
    return self.execute("INSTALL_ADDON", payload)["value"]
.tox/ui-tests/lib/python3.6/site-packages/selenium/webdriver/remote/webdriver.py:308: in execute
    self.error_handler.check_response(response)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <selenium.webdriver.remote.errorhandler.ErrorHandler object at 0x1040c2c50>
response = {'status': 500, 'value': '{"value":{"error":"unknown error","message":"[Exception... \\"Component returned failure cod...ad_start::hba7593f2e1f1beb1\\n  10:     0x7fff53e7a6c0 - _pthread_body\\n  11:     0x7fff53e7a56c - _pthread_start"}}'}

    def check_response(self, response):
        """
            Checks that a JSON response from the WebDriver does not have an error.

            :Args:
             - response - The JSON response from the WebDriver server as a dictionary
               object.

            :Raises: If the response contains an error message.
            """
        status = response.get('status', None)
        if status is None or status == ErrorCode.SUCCESS:
            return
        value = None
        message = response.get("message", "")
        screen = response.get("screen", "")
        stacktrace = None
        if isinstance(status, int):
            value_json = response.get('value', None)
            if value_json and isinstance(value_json, basestring):
                import json
                try:
                    value = json.loads(value_json)
                    if len(value.keys()) == 1:
                        value = value['value']
                    status = value.get('error', None)
                    if status is None:
                        status = value["status"]
                        message = value["value"]
                        if not isinstance(message, basestring):
                            value = message
                            message = message.get('message')
                    else:
                        message = value.get('message', None)
                except ValueError:
                    pass

        exception_class = ErrorInResponseException
        if status in ErrorCode.NO_SUCH_ELEMENT:
            exception_class = NoSuchElementException
        elif status in ErrorCode.NO_SUCH_FRAME:
            exception_class = NoSuchFrameException
        elif status in ErrorCode.NO_SUCH_WINDOW:
            exception_class = NoSuchWindowException
        elif status in ErrorCode.STALE_ELEMENT_REFERENCE:
            exception_class = StaleElementReferenceException
        elif status in ErrorCode.ELEMENT_NOT_VISIBLE:
            exception_class = ElementNotVisibleException
        elif status in ErrorCode.INVALID_ELEMENT_STATE:
            exception_class = InvalidElementStateException
        elif status in ErrorCode.INVALID_SELECTOR \
                or status in ErrorCode.INVALID_XPATH_SELECTOR \
                or status in ErrorCode.INVALID_XPATH_SELECTOR_RETURN_TYPER:
            exception_class = InvalidSelectorException
        elif status in ErrorCode.ELEMENT_IS_NOT_SELECTABLE:
            exception_class = ElementNotSelectableException
        elif status in ErrorCode.ELEMENT_NOT_INTERACTABLE:
            exception_class = ElementNotInteractableException
        elif status in ErrorCode.INVALID_COOKIE_DOMAIN:
            exception_class = WebDriverException
        elif status in ErrorCode.UNABLE_TO_SET_COOKIE:
            exception_class = WebDriverException
        elif status in ErrorCode.TIMEOUT:
            exception_class = TimeoutException
        elif status in ErrorCode.SCRIPT_TIMEOUT:
            exception_class = TimeoutException
        elif status in ErrorCode.UNKNOWN_ERROR:
            exception_class = WebDriverException
        elif status in ErrorCode.UNEXPECTED_ALERT_OPEN:
            exception_class = UnexpectedAlertPresentException
        elif status in ErrorCode.NO_ALERT_OPEN:
            exception_class = NoAlertPresentException
        elif status in ErrorCode.IME_NOT_AVAILABLE:
            exception_class = ImeNotAvailableException
        elif status in ErrorCode.IME_ENGINE_ACTIVATION_FAILED:
            exception_class = ImeActivationFailedException
        elif status in ErrorCode.MOVE_TARGET_OUT_OF_BOUNDS:
            exception_class = MoveTargetOutOfBoundsException
        else:
            exception_class = WebDriverException
        if value == '' or value is None:
            value = response['value']
        if isinstance(value, basestring):
            if exception_class == ErrorInResponseException:
                raise exception_class(response, value)
            raise exception_class(value)
        if message == "" and 'message' in value:
            message = value['message']

        screen = None
        if 'screen' in value:
            screen = value['screen']

        stacktrace = None
        if 'stackTrace' in value and value['stackTrace']:
            stacktrace = []
            try:
                for frame in value['stackTrace']:
                    line = self._value_or_default(frame, 'lineNumber', '')
                    file = self._value_or_default(frame, 'fileName', '<anonymous>')
                    if line:
                        file = "%s:%s" % (file, line)
                    meth = self._value_or_default(frame, 'methodName', '<anonymous>')
                    if 'className' in frame:
                        meth = "%s.%s" % (frame['className'], meth)
                    msg = "    at %s (%s)"
                    msg = msg % (meth, file)
                    stacktrace.append(msg)
            except TypeError:
                pass
        if exception_class == ErrorInResponseException:
            raise exception_class(response, message)
        elif exception_class == UnexpectedAlertPresentException and 'alert' in value:
            raise exception_class(message, screen, stacktrace, value['alert'].get('text'))
>       raise exception_class(message, screen, stacktrace)
E       selenium.common.exceptions.WebDriverException: Message: [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIFile.initWithPath]"  nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame :: chrome://marionette/content/addon.js :: addon.install :: line 91"  data: no]

.tox/ui-tests/lib/python3.6/site-packages/selenium/webdriver/remote/errorhandler.py:194: WebDriverException

It seems similar to bug 1389488 but fails at a different point, around here: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/addon.js#91
b4hand: Are you interested in writing a fix and a test for this?
(In reply to Dave Hunt (:davehunt) from comment #1)
> b4hand: Are you interested in writing a fix and a test for this?

Yes I am. Any ideas? Maybe a try catch kind of, but in JavaScript of course.
b4hand: I would start with seeing if you can create a failing test case. A try/catch might be a suitable fix, however :whimboo or :ato may have a better suggestion.
Assignee: nobody → bforehand
Try/catch is fine with me, and we might want to combine it with a possible failure as result of calling `file.exists()`
Summary: Addon,js install fails if path is incorrect → Addon.js install fails if path is incorrect
Comment on attachment 8920293 [details]
Bug 1408066 - Addon.js install fails if path is incorrect.

https://reviewboard.mozilla.org/r/191316/#review196498

Sorry for picking up the review from you Dave. But I thought this way Benny has the time to update the patch until you are around tomorrow.

::: testing/marionette/addon.js:96
(Diff revision 1)
>    let addon;
>  
> +  try {
> +    let file = new FileUtils.File(path);
> -  if (!file.exists()) {
> +    if (!file.exists()) {
> -    throw new UnknownError(`Could not find add-on at '${path}'`);
> +      throw new UnknownError();

Please only wrap the constructor of `File` into the try/catch, and use a better error message for that case. Right now we would loose the stack frame in case the file does not exist.

::: testing/marionette/addon.js:103
(Diff revision 1)
> +  } catch (e) {
> +    throw new UnknownError(
> +        `Could not find add-on at '${path}'`);
>    }
>  
> +  let file = new FileUtils.File(path);

Why do you add this line here again? You have it already above, so make use of the variable after moving its declaration out of the try/catch block.

::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:102
(Diff revision 1)
>  
>          with self.assertRaisesRegexp(AddonInstallException, "Could not find add-on at"):
>              self.addons.install(addon_path)
> +
> +    def test_install_incorrect_path(self):
> +        addon_path = os.path.join('bad/path', 'does-not-exist.xpi')

Just use the plain xpi file name here. This is also a relative path and will cause an error to be thrown. Maybe update the test name to "test_install_with_relative_path"
Attachment #8920293 - Flags: review-
(In reply to Henrik Skupin (:whimboo) from comment #7)
> 
> ::: testing/marionette/addon.js:103
> (Diff revision 1)
> > +  } catch (e) {
> > +    throw new UnknownError(
> > +        `Could not find add-on at '${path}'`);
> >    }
> >  
> > +  let file = new FileUtils.File(path);
> 
> Why do you add this line here again? You have it already above, so make use
> of the variable after moving its declaration out of the try/catch block.
> 

It was my understanding that a the variables defined within a try/catch are only available within that scope.
(In reply to Benjamin Forehand Jr[:b4hand] from comment #8)
> It was my understanding that a the variables defined within a try/catch are
> only available within that scope.

That's why I said "after moving its declaration out of the try/catch block". :)

Also please note that when working with mozreview you should best reply inside this tool by navigating to the created issue first. Doing that will keep all the comments together.
You also created a complete new commit in that patch series. I wonder who that is possible at all. Please check that and if necessary fixup/squash all the commits into a single one.
Flags: needinfo?(bforehand)
(In reply to Henrik Skupin (:whimboo) from comment #11)
> You also created a complete new commit in that patch series. I wonder who
> that is possible at all. Please check that and if necessary fixup/squash all
> the commits into a single one.

Ah yes...I apologize. First time using git cinnabar. I'll fix it.
Flags: needinfo?(bforehand)
Attachment #8920370 - Attachment is obsolete: true
Attachment #8920370 - Flags: review?(dave.hunt)
Comment on attachment 8920293 [details]
Bug 1408066 - Addon.js install fails if path is incorrect.

https://reviewboard.mozilla.org/r/191316/#review196642

::: testing/marionette/addon.js:98
(Diff revision 2)
> +
> +  try {
> +    file = new FileUtils.File(path);
> +  } catch (e) {
> +    throw new UnknownError(
> +        `Bad path. Please provide the full path to the add-on.`);

It's not a bad path, but not an absolute one. See the documentation on MDN:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsILocalFile#initWithPath()

So a better error message would be:

"An absolute path is required for the addon."

::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:101
(Diff revision 2)
>          addon_path = os.path.join(here, "does-not-exist.xpi")
>  
>          with self.assertRaisesRegexp(AddonInstallException, "Could not find add-on at"):
>              self.addons.install(addon_path)
> +
> +    def test_install_incorrect_relative_path(self):

I think you missed my review comments from the former review.
Comment on attachment 8920293 [details]
Bug 1408066 - Addon.js install fails if path is incorrect.

https://reviewboard.mozilla.org/r/191316/#review196788

::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:101
(Diff revision 2)
>          addon_path = os.path.join(here, "does-not-exist.xpi")
>  
>          with self.assertRaisesRegexp(AddonInstallException, "Could not find add-on at"):
>              self.addons.install(addon_path)
> +
> +    def test_install_incorrect_relative_path(self):

I didn't miss them but it was a bit difficult to understand what you meant from just 1 sentence. 

This should be ```test_install_relative_path```? The first augument for ```os.join``` could be something like ```'relative-path'```, and then of course the new error message.
Comment on attachment 8920293 [details]
Bug 1408066 - Addon.js install fails if path is incorrect.

https://reviewboard.mozilla.org/r/191316/#review196788

> I didn't miss them but it was a bit difficult to understand what you meant from just 1 sentence. 
> 
> This should be ```test_install_relative_path```? The first augument for ```os.join``` could be something like ```'relative-path'```, and then of course the new error message.

Henrik meant that a more accurate name for the test would be "test_install_with_relative_path". He's also suggesting that you can recreate a relative path by simply specifying self.addons.install("does-not-exist.xpi"). There is no need to use os.join at all.
Comment on attachment 8920293 [details]
Bug 1408066 - Addon.js install fails if path is incorrect.

https://reviewboard.mozilla.org/r/191316/#review196788

> Henrik meant that a more accurate name for the test would be "test_install_with_relative_path". He's also suggesting that you can recreate a relative path by simply specifying self.addons.install("does-not-exist.xpi"). There is no need to use os.join at all.

It would be better to reference an existing addon, to not actually overlap with the path not exist failure. But as I can see, Benjamin did that with the latest update.
Comment on attachment 8920293 [details]
Bug 1408066 - Addon.js install fails if path is incorrect.

https://reviewboard.mozilla.org/r/191316/#review197726

::: testing/marionette/addon.js:92
(Diff revision 5)
>   *     If there is a problem installing the addon.
>   */
>  addon.install = async function(path, temporary = false) {
> -  let file = new FileUtils.File(path);
>    let addon;
> +  var file;

Why switch from let to var?

::: testing/marionette/addon.js:98
(Diff revision 5)
> +
> +  try {
> +    file = new FileUtils.File(path);
> +  } catch (e) {
> +    throw new UnknownError(
> +        `Please provide the absolute path to the add-on.`);

Indentation should match the rest of the file.
Attachment #8920293 - Flags: review?(dave.hunt) → review-
Comment on attachment 8920293 [details]
Bug 1408066 - Addon.js install fails if path is incorrect.

https://reviewboard.mozilla.org/r/191316/#review197726

> Why switch from let to var?

Hmm my knowledge of JS6 has failed me. I thought since we will access this variable later it needed to be a ```var``` and not a ```let```. Works with ```let``` though.
Comment on attachment 8920293 [details]
Bug 1408066 - Addon.js install fails if path is incorrect.

https://reviewboard.mozilla.org/r/191316/#review198270

r=me with the comments fixed.

::: testing/marionette/addon.js:98
(Diff revision 6)
> +
> +  try {
> +    file = new FileUtils.File(path);
> +  } catch (e) {
> +    throw new UnknownError(
> +        `Please provide the absolute path to the add-on.`);

Exception messages should not contain a proposal how to get it fixed, but explain what was wrong. Also you are not adding the addon path anymore.

Here an example: `${path} is not an absolute path.`

If the length of the line is shorter then 79, then leave all in one single line.

::: testing/marionette/addon.js:103
(Diff revision 6)
> +        `Please provide the absolute path to the add-on.`);
> +  }
>  
>    if (!file.exists()) {
> -    throw new UnknownError(`Could not find add-on at '${path}'`);
> +    throw new UnknownError(
> +        `Could not find add-on at '${path}'`);

This change is unnecessary, and not needed because the line length doesn't exceed the maximum amount of characters.
Attachment #8920293 - Flags: review+
Thanks Benjamin for the update. I just pushed a try build. If that works fine we can get your patch landed.
Status: NEW → ASSIGNED
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/458f149ce413
Addon.js install fails if path is incorrect. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/458f149ce413
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.