1216704 - chrome.runtime.getManifest() fails from a content script with error message "TypeError: context is undefined"

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Jesper Kristensen]

STR
Call chrome.runtime.getManifest() from a WebExtension content script

Actual results:
An exception is thrown: "TypeError: context is undefined"

It links to resource://gre/modules/ExtensionContent.jsm line 64

The code is this:

    getManifest: function(context) {
      return context.extension.getManifest();
    },


I think it should be this:

    getManifest: function() {
      return context.extension.getManifest();
    },

That is, the method should have no parameters.

Comment 1

[jignesh]

Attachment: 1216704.patch

Hi,

This is my first bug, I have done the change as suggested in the first comment,
It built successfully, although I didnt test it (because I don't know how to).

Please take a look.

Comment 2

[Johann Hofmann [:johannh]]

Hi jignesh! Thanks for the patch!

One idea could be to extend this file https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_contentscript.html with a test if getManifest() returns the correct data.

More info on Mochitest here: https://developer.mozilla.org/en/docs/Mochitest

You have flagged Jesper for review, I'm not sure if Jesper can review this component though.

cc :billm, would you like to review this after tests?

Comment 3

[Jesper Kristensen]

Comment on attachment 8686011 [details] [diff] [review]
1216704.patch

I am not a reviewer

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, I can review this once we have a test. I think it should be enough to call "browser.runtime.getManifest()" right before this line:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_contentscript.html#42

You can run the test using the instructions Johann provided. Basically, you need to run:
  mach mochitest test_ext_contentscript.html
from the command line. It should tell you some tests passed and none failed.

Comment 5

[jignesh]

Attachment: 1216704.patch

I have added the test as suggested. Although I don't get how just calling getManifest() alone inside contentScript() does the job. Can anyone explain this or point to some resource explaining the test framework?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8686708 [details] [diff] [review]
1216704.patch

Review of attachment 8686708 [details] [diff] [review]:
-----------------------------------------------------------------

Hi jignesh. I tried your test and it looks like there's a bit more work to do. Thanks for the help though. With these new changes everything should work. Please post a revised patch and ask me for review again.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +64,1 @@
>        return context.extension.getManifest();

I just tried running the test myself and it still fails even with your change. It works if we change this line to be the same as the implementation for non-content scripts (in ext-runtime.js):
      return Cu.cloneInto(context.extension.manifest, context.cloneScope);
This gets the correct manifest and clones it into a compartment that is accessible to extension code (which runs in a separate security context).

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript.html
@@ +38,5 @@
>      browser.runtime.sendMessage(["script-run", "complete", document.readyState]);
>    }
>  
>    function contentScript() {
> +    browser.runtime.getManifest();

If you call this function without the changes to ExtensionContent.jsm, it will throw an exception. Therefore, the sendMessage call on the next line will never run. That means that the chromeNamespacePromise promise will never resolve, so the test will sit forever waiting for it.

You can try this out by running the test without the changes to ExtensionContent.jsm. It should never finish, which counts as a failure.

It would also be nice to check the manifest actually has valid data in it. You can do that by replacing this line with the following:
    var manifest = browser.runtime.getManifest();
    manifest.applications.gecko.id;
That way, if there's no "applications" property in the manifest, the test will throw an exception and never send the chrome-namespace-ok message.

Comment 7

[jignesh]

Attachment: 1216704.patch

Thanks Bill for the review, have done the changes.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8687661 [details] [diff] [review]
1216704.patch

Review of attachment 8687661 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Are you familiar with the checkin procedure?

Comment 9

[jignesh]

According to the intro doc, I need to submit a patch and get it reviewed. Then someone with write access to source code will checkin the patch on my behalf. 

Anything else need to be done? Patch format not proper? Please let me know.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Please update the commit message to say something like "Bug 1216704 - [webext] Fix incorrect parameter to getManifest in content scripts (r=billm)". Then attach the new patch to the bug, mark the old patch obsolete, and set the checkin-needed keyword on the bug.

Comment 11

[jignesh]

Attachment: 1216704.patch

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ec5135f71a22

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/ec5135f71a22

Comment 14

[Amy Tsay [:amyt]]

Thanks Jignesh! I've added your contribution to https://wiki.mozilla.org/Add-ons/Contribute/Recognition#November_2015