Closed Bug 1216704 Opened 9 years ago Closed 9 years ago

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


(WebExtensions :: Untriaged, defect)

Not set


(firefox45 fixed)

45.2 - Nov 30
Tracking Status
firefox45 --- fixed


(Reporter: bugzilla, Assigned: jigneshjain25)


(Whiteboard: [good first bug])


(1 file, 3 obsolete files)

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.
Whiteboard: [good first bug]
Attached patch 1216704.patch (obsolete) — Splinter Review

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.
Attachment #8686011 - Flags: review?(bugzilla)
Hi jignesh! Thanks for the patch!

One idea could be to extend this file with a test if getManifest() returns the correct data.

More info on Mochitest here:

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?
Flags: needinfo?(wmccloskey)
Assignee: nobody → jigneshjain25
Comment on attachment 8686011 [details] [diff] [review]

I am not a reviewer
Attachment #8686011 - Flags: review?(bugzilla)
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:

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.
Flags: needinfo?(wmccloskey)
Attached patch 1216704.patch (obsolete) — Splinter Review
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?
Attachment #8686011 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Attachment #8686708 - Flags: review?(wmccloskey)
Comment on attachment 8686708 [details] [diff] [review]

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();;
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.
Attachment #8686708 - Flags: review?(wmccloskey)
Flags: needinfo?(wmccloskey)
Attached patch 1216704.patch (obsolete) — Splinter Review
Thanks Bill for the review, have done the changes.
Attachment #8686708 - Attachment is obsolete: true
Attachment #8687661 - Flags: review?(wmccloskey)
Comment on attachment 8687661 [details] [diff] [review]

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

Thanks! Are you familiar with the checkin procedure?
Attachment #8687661 - Flags: review?(wmccloskey) → review+
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.
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.
Attached patch 1216704.patchSplinter Review
Attachment #8687661 - Attachment is obsolete: true
Attachment #8689171 - Flags: checkin+
Attachment #8689171 - Flags: checkin+ → checkin?(wmccloskey)
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Attachment #8689171 - Flags: checkin?(wmccloskey) → checkin+
Iteration: --- → 45.2 - Nov 30
Thanks Jignesh! I've added your contribution to
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.