395430 - AdBlock Plus gets installed but is marked as incompatible after restart

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[u279076]

Using Minefield 2007090704 on Linux, I can install AdBlock Plus from AMO but when I restart Minefield, the addon is marked as incompatible but no errors were given at all.

Steps to Reproduce:
1. Go to . and click on "install now" button
2. click install now from the Software Installation UI
3. AM comes up trying to install adblock
4. it downloads and checks compatibility
5. text below addon says "restart to complete installation"
6. click "restart minefield", minefield is restarted
7. after minefield comes back up, it behaves as if no addon was installed
8. check AM

Actual results:
Adblock says "not compatible with 3.0a8pre" with a button to uninstall.  No errors are displayed throughout.  Click on uninstall works as expected.

Expected results:
If the addon is indeed incompatible, this should be found during the compatibility check.

Comment 1

[u279076]

sorry...step 1 should read:

1. Go to AMO, find the AdBlock Plus addon page and click on "install now" button

Comment 2

[Dave Townsend [:mossop]]

I know what's causing this, have a patch in progress.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

So we install the extension, detect new compatibility info to the datasource to be used on startup.

On restart we retrieve the updated compatibility info from the datasource, but since we fail to return the appid for the updated info, when we pass that back in to updateTargetAppInfo it does nothing with the new info, leaving the extension incompatible and disabled.

Comment 4

[Dave Townsend [:mossop]]

This is a regression from bug 299716. I have a testcase that is close to covering this that I can adapt but for the moment would like to try to get this fix into the tree a.s.a.p.

Comment 5

[Alex Vincent [:WeirdAl]]

I'd r+ that in a heartbeat if so empowered.  Pretty obvious.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 280108 [details] [diff] [review]
patch rev 1

Requesting approval for this extremely simple patch that corrects extension installation in certain cases. There is I believe a pretty much non-existent risk of regressions from this

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 280108 [details] [diff] [review]
patch rev 1

yeah, let's get this in...

Comment 8

[Dave Townsend [:mossop]]

Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.253; previous revision: 1.252
done

Fixed, will pick up the test later. Thanks for spotting this Anthony.

Comment 9

[Dave Townsend [:mossop]]

Attachment: testcase rev 1

This is a fairly simple testcase, installs 2 extensions both initially incompatible. The update.rdf should make one compatible and not the other. On restart check that one is installed and enabled and the other is not installed.

Minor wrinkle is the need for a timeout to wait for the compatibility check to complete, can't figure out a decent way to detect that the EM is done.

Comment 10

[Marcia Knous [:marcia]]

Litmus Triage Team: We may need to re-evaluate the need for this. robcee - what are the plans for automating this testing?

Comment 11

[Robert Strong (they/them - no direct email)]

Perhaps a couple of new onStateChange values (INCOMPATIBLE_UPDATE_START and INCOMPATIBLE_UPDATE_DONE) could be added to nsIAddonUpdateListener and implement nsIAddonUpdateListener in the test? A hacky approach that should work:
for INCOMPATIBLE_UPDATE_START increment gInstallCount
for INCOMPATIBLE_UPDATE_DONE decrement gInstallCount
check gInstallCount == 0 after the switch
ignore DIALOG_CLOSE as we do DOWNLOAD_START, DOWNLOAD_DONE, and INSTALL_START in extensions.js

This would also solve the bug where the restart button is enabled before the incompatible update check is finished during normal operations as well as when installing updates during startup.

Thoughts?

Comment 12

[Robert Strong (they/them - no direct email)]

Dave, I threw together a patch to implement what I suggested in comment #11... I'll file a bug and submit it later.

Comment 13

[Dave Townsend [:mossop]]

Cheers Rob, seems like a reasonable way to go

Comment 14

[Rob Campbell [:rc] (:robcee)]

marcia: we don't have any current plans to test correct installation of add-ons yet. That might be a good candidate for a test suite for next quarter.

Comment 15

[Robert Strong (they/them - no direct email)]

Filed Bug 396129 for the additional states for nsIAddonUpdateListener

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 280453 [details] [diff] [review]
testcase rev 1

Dropping the request till bug 396129 is resolved one way or another

Comment 17

[Tony Chung [:tchung]]

Hi dave, this bug is marked RESOLVED FIXED, but i dont see the checkin yet.  When do you anticipate this landing?   Thanks.

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #17)
> Hi dave, this bug is marked RESOLVED FIXED, but i dont see the checkin yet. 
> When do you anticipate this landing?   Thanks.


https://bugzilla.mozilla.org/show_bug.cgi?id=395430#c8

Comment 19

[Marcia Knous [:marcia]]

Litmus triage team: tchung will handle this litmus testcase

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Version 0.7.5.3 from September 24, 2007, downloaded from:

https://addons.mozilla.org/en-US/firefox/addon/1865

on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011104 Minefield/3.0b3pre

Comment 21

[Tony Chung [:tchung]]

in-litmus flag cleanup.  
per comment 9, there are two tests already in-testsuite that covers this bug.

Comment 22

[Dave Townsend [:mossop]]

test_install.js verifies this scenario now