Cached file is used while installing local WebExtensions package after modification.

RESOLVED FIXED in Firefox 55

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: arai, Assigned: swapneshks, Mentored)

Tracking

({good-first-bug})

53 Branch
mozilla55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Steps to reproduce:
  1. create a WebExtensions package with a syntax error in manifest.json
  2. drag and drop the file onto Add-on manager
  3. confirm the error message about the corruption
  4. fix the syntax error in manifest.json and re-pack, with same filename
  5. drag and drop the file onto Add-on manager

Actual result:
  add-on manager says the file is corrupted, and error for the previous syntax error is printed on browser console again

Expected result:
  installed properly

Restarting Firefox or renaming the file solves the issue.
that sounds like it's using cache.
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
this still needs to be looked into
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Updated

2 years ago
Mentor: aswan
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: triaged
Hi.. Can I work on this bug?
I'm a beginner and catching up to things...
Flags: needinfo?(aswan)
Sure, thanks for helping out!

The first step, if you haven't done it is to download and build Firefox:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
For this bug, an artifact build will be fine, take a look at this page as well before you start building:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

At the same time, also follow the steps from the original report to ensure you can reproduce the bug.

When you're ready to actually tackle it, I think you want to add a call to flushJarCache() if there is a failure here:
http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6110-6121
After adding that, the original steps for reproduction should no longer cause the bug.

If you manage to get through all of that, then we can talk about what sort of tests need to be added along with the fix.  And of course feel free to ask here or on IRC if you get stuck or have any questions along the way.

Good luck and thanks again!
Assignee: nobody → swapneshks
Flags: needinfo?(aswan)
Sorry for the delayed response. After applying the changes in the above patch, the issue described in this bug is not seen in my local build. If the patch seems alright, then I can help proceed with the tests to be added. :)
Attachment #8863117 - Flags: review?(aswan)
Following from Comment#4, the change that I made is only to line#6147. I am not sure why line#5985 is in the diff.
Comment on attachment 8863117 [details] [diff] [review]
Flush cache after WebExtensions XPI load fails

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

Great, thanks!
Just to confirm, you reproduced the original bug, applied your changes, then followed the same steps and didn't see the bug?
That's excellent, but we still need tests before this can land (and the unrelated change in your patch needs to be removed).

This should be testable using xpcshell.  There is documentation on MDN but it appears to be a little dated.  Also, toolkit/mozapps/extensions/test/xpcshell has a lot of tests, some of which are quite old and don't follow our current practices.  I would suggest starting with something like test_multiprocessCompatible.js as a template.  The only actual code bits you'll need to borrow from it are the code to create new extensions (createTempXPIFile) and to start installations (AddonManager.getInstallForFile, etc).  The outline of the test would be something like:
1. create a webextension xpi that contains a syntax error
2. verify that attempting to install it fails
3. replace the file with a valid webextension
4. verify that installing the valid extension from the same path now succeeds.

Then, your test should fail without your patch applied and pass with it applied.

Learning your way around the test framework can be a little tricky, please feel free to ask any questions that come up.
Thanks again, looking forward to seeing this land!
Attachment #8863117 - Flags: review?(aswan)
Hi Andrew,

So I wrote a xpcshell test having the path as mozilla-central/toolkit/mozapps/extensions/test/xpcshell/test_bug1345007.js. After creating the test, I added the test to xpcshell-shared.ini (where the other bug tests are added).

Contents of the test are at https://pastebin.mozilla.org/9020546. (Currently addonDataWithError does not have any syntax error)

As it can be seen, I've intentionally added ok(false) in the test. However, the test always passes, whatsoever I do. (Unless there is a syntax error in the test itself.) For instance, I have tried 
>  do_check_true(isExtensionInAddonsList(profileDir, addonDataWithoutError.id))
without creating XPI file for addonDataWithoutError but the test passed when it should have failed.

It seems that either I am using deprecated function calls or not including some modules, but I can't seem to find a reference. Could you please guide me regarding the same?
Flags: needinfo?(aswan)
From a quick glance, you should have gotten a javascript parsing error since you're using yield inside a regular (non-generator) function.
In any case, the preferred style now is to put individual test cases into separate async functions that are registered with add_task().  I think you should be able to move most of your existing code.  Also, you don't need to restart the addons manager after installing, the install should fail (so you'll presumably need a try {} block around it.  Also note that when you create the second xpi file, you'll want to use the same path you used for the first one, you could call createTempXPIFile and then rename it or you can just use AddonTestUtils.writeFilesToZip() directly.

Looking good though, thanks for your efforts!
Flags: needinfo?(aswan)
Posted file test_bug1345007_v2.zip (obsolete) —
It took me quite some time and effort to get the test to work, but the effort paid off in the end. The test and log files are in the attached ZIP file. 

There were some issues (invalid manifest file) while using AddonTestUtils.writeFilesToZip() so I went ahead with the renaming approach instead.

The test is producing expected output. However, there are 2 things to note:
1. For an invalid XPI file, the test outputs a warning and not an error (without any exception) and so the try..catch block does not have any effect. Is there any way to convert a WARN to an ERROR?
2. Somehow, after the second install, the install output (i.e. variable install2) is not null. In this case, how do I make the test produce a fail signature (since the errors generated are javascript errors)?

Once we resolve the above 2 issues, I'll add more comments in the test and make the test output more verbose before we commit and push the test. :)
Flags: needinfo?(aswan)
Looking good, thanks for all the hard work so far.
First of all, please just attach patche directly to the bug, it makes it easier to view and comment on the diffs (you can include the logs in separate attachments if you like).
As for the specific questions:

(In reply to Swapnesh Kumar Sahoo [:swapneshks] from comment #9)
> 1. For an invalid XPI file, the test outputs a warning and not an error
> (without any exception) and so the try..catch block does not have any
> effect. Is there any way to convert a WARN to an ERROR?

You should be able to just check install1.state and install1.error, ie something like (untested):
do_check_eq(install1.state, AddonManager.STATE_DOWNLOAD_FAILED, "Install failed");
do_check_eq(install1.error, AddonManager.ERROR_CORRUPT_FILE, "Install error is for corrupt file");

> 2. Somehow, after the second install, the install output (i.e. variable
> install2) is not null. In this case, how do I make the test produce a fail
> signature (since the errors generated are javascript errors)?

I'm not sure what you mean by "make the test produce a fail signature", you should be able to call install2.install() then wait for it to complete.  Actually, a better way to do that is to just call promiseInstallFile() instead of AddonManager.getInstallForFile().
Flags: needinfo?(aswan)
Sorry for not attaching the test as a patch. It was an add so I was not sure how to attach it. For the test, I went ahead with getInstallForFile() as it expands the scope of error checking. The test is now producing expected output. I am attaching the logs separately. 
As for XPIProvider.jsm, I resolved the unrelated change (Comment#5). :)
Do let me know if the patch seems fine.
Attachment #8866049 - Flags: review?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8863117 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8864989 - Attachment is obsolete: true
Attachment #8866052 - Flags: feedback?(aswan)
Attachment #8866053 - Flags: feedback?(aswan)
Comment on attachment 8866049 [details] [diff] [review]
patch_v2 : Flush cache after WebExtensions XPI load fails

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

We're getting close, thanks for your efforts.
A few things:
- The test should not just print out its results, it needs to use the functions described here so the pass/failure status propagates propertly: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging
- The test_bugNNN.js naming convention is obsolete, please give the file a descriptive name, e.g. test_install_error.js or something
- Please run eslint over the test file, there are a few things that are inconsistent with our conventions.  You should be able to run "mach eslint toolkit/mozapps/extension/test/xpcshell/test_xxx.js" from your source tree

Thanks!
Attachment #8866049 - Flags: review?(aswan) → review-
This patch contains changes as per comment#14.
I ran eslint over the test file. There were few errors related to spaces and braces which have been fixed in this patch.
Also, in the manifest, skip for thunderbird and tag have been added.
Attachment #8866483 - Flags: review?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8866049 - Attachment is obsolete: true
Comment on attachment 8866483 [details] [diff] [review]
Flush cache after WebExtensions XPI load fails

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

Excellent, we are nearly there.  There are a few nits, then the last thing is to fix the commit message.  The first line should be something like:
Bug 1345007 Flush jar cache after failed installation
You can include further detail below that if you like but I think that summary is probably adequate.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install_syntax_error.js
@@ +1,5 @@
> +const ADDON_ID = "webext-test@tests.mozilla.org";
> +
> +add_task(async function install_xpi() {
> +
> +  Services.prefs.setBoolPref("extensions.allow-non-mpc-extensions", true);

this should be unneeded, please remove it

@@ +8,5 @@
> +  let xpi1 = Extension.generateXPI({
> +    files: {
> +      "manifest.json": String.raw`{
> +        // This is a manifest. Intentional syntax error in next line.
> +        "manifest_version: 2,  

remove trailing whitespace

@@ +11,5 @@
> +        // This is a manifest. Intentional syntax error in next line.
> +        "manifest_version: 2,  
> +        "applications": {"gecko": {"id": "${ADDON_ID}"}},
> +        "name": "Temp WebExt with Error",
> +        "version": "0.1" 

remove trailing whitespace

@@ +24,5 @@
> +        // This is a manifest.
> +        "manifest_version": 2,
> +        "applications": {"gecko": {"id": "${ADDON_ID}"}},
> +        "name": "Temp WebExt without Error",
> +        "version": "0.1" 

remove trailing whitespace

@@ +32,5 @@
> +
> +  let install1 = await AddonManager.getInstallForFile(xpi1);
> +  do_check_eq(install1.state, AddonManager.STATE_DOWNLOAD_FAILED);
> +  do_check_eq(install1.error, AddonManager.ERROR_CORRUPT_FILE);
> +  if (install1.state == AddonManager.STATE_DOWNLOAD_FAILED) {

this whole block is unnecessary, please remove it

@@ +46,5 @@
> +  xpi2.moveTo(xpi1.parent, xpi1.leafName);
> +
> +  let install2 = await AddonManager.getInstallForFile(xpi2);
> +  do_check_neq(install2.error, AddonManager.ERROR_CORRUPT_FILE);
> +  if (install2.state == AddonManager.STATE_DOWNLOAD_FAILED) {

this whole block is unnecessary, please remove it

@@ +55,5 @@
> +      do_print("Install error due to other unexpected reason.");
> +    }
> +  }
> +
> +  Services.prefs.clearUserPref("extensions.allow-non-mpc-extensions");

this should be removed too

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini
@@ +362,4 @@
>  [test_ext_management.js]
>  skip-if = appname == "thunderbird"
>  tags = webextensions
> +[test_webextension_install_syntax_error.js]

Please put this in the right place in the alphabetized section above
Attachment #8866483 - Flags: review?(aswan)
Changes incorporated as per comment#16
Attachment #8866790 - Flags: review?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8866483 - Attachment is obsolete: true
Comment on attachment 8866790 [details] [diff] [review]
Flush jar cache after failed installation

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

Great, thank you for all the hard word and persistence!
I've started a try run to make sure this test passes on all platforms, assuming this is successful we can land the patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=19ddfacdaa411285d90f0d5885b423a033a42514
Attachment #8866790 - Flags: review?(aswan) → review+
It was great learning experience. Thanks for your guidance throughout!
Okay, the try backlog for osx is ridiculous today.  Given that the test passed on linux and windows, I think this is good to land.
Swapnesh, do you want to do the honors?  You can set the keyword checkin-needed, and a sheriff will land the patch in one of th integration branches.  Once it passes automated tests there, it will get merged to mozilla-central and the bug will get resolved at that time.
Thanks again for all your hard work, I'd be happy to work with you on additional bugs if you're interested in building on everything you learned fixing this bug!
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(In reply to Andrew Swan [:aswan] from comment #20)

> Thanks again for all your hard work, I'd be happy to work with you on
> additional bugs if you're interested in building on everything you learned
> fixing this bug!

My pleasure! 
I will be more than happy to help and work on additional bugs! I am reachable via the same email ID mentioned in my bugzilla profile. :)
Hi Swapnesh! Thanks so much for fixing this bug! Your contribution has been added to our recognition wiki here: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#May_2017

If you're interested, please feel welcome to set up a profile on mozillians.org -- I would be happy to vouch for your contributions. 

Thanks again, and welcome onboard! We would love your help with some of these other good first bugs: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=component%3AWebExtensions%20keyword%3Agood-first-bug&list_id=13581837
(In reply to Caitlin Neiman (http://pronoun.is/she) from comment #22)

> Hi Swapnesh! Thanks so much for fixing this bug! Your contribution has been
> added to our recognition wiki here:
> https://wiki.mozilla.org/Add-ons/Contribute/Recognition#May_2017

Wow! This is great! Thanks for letting me know.
 
> If you're interested, please feel welcome to set up a profile on
> mozillians.org -- I would be happy to vouch for your contributions. 

Thanks Caitlin, that's really nice of you. :)
I've set up a mozillians.org profile - https://mozillians.org/en-US/u/swapneshks/

> Thanks again, and welcome onboard! 

Your welcome.. I don't know how to thank everybody here for their support!

> We would love your help with some of these other good first bugs:
> https://bugzilla.mozilla.org/buglist.cgi?quicksearch=component%3AWebExtensions%20keyword%3Agood-first-bug&list_id=13581837

I am currently working on few other moz bugs but once those are fixed, I will definitely work on bugs in the above list!
\o/ 

You're vouched! Good luck with your other bugs! We will look forward to seeing you again soon!

Comment 25

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0a1be408ae
Flush jar cache after failed installation. r=aswan
Keywords: checkin-needed
(In reply to Caitlin Neiman (http://pronoun.is/she) from comment #24)
> \o/ 
> 
> You're vouched! 

Thank you so much, Caitlin!

> We will look forward to seeing you again soon!

Same here! :)
https://hg.mozilla.org/mozilla-central/rev/9d0a1be408ae
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attachment #8866052 - Flags: feedback?(aswan)
Attachment #8866053 - Flags: feedback?(aswan)
You need to log in before you can comment on or make changes to this bug.