723012 - simple-storage broken in Addon kit 1.4.2

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Thomas Bertels]

Say that we have an extension with id "myname@something".

With Addon kit 1.4.2, the storage dir is in
[profilename]\jetpack\myname\simple-storage
instead of
[profilename]\jetpack\myname@something\simple-storage

And so the simple-storage module doesn't seem to able to do its job.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Hi Tomas, thanks for the report!

Can I look at your add-on ? What's id does it has in package.json ?

I'm testing against 1.4.2 and simple storage saves into:

\jetpack\[e-mail]\simple-storage\store.json

Also what was a previous version of SDK where your add-on worked as expected ?

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I also have tested with 1.3 and behavior seems to be the same as 1.4.2 and current master. So I'm marking it as unconfirmed until we get a further input.

Comment 3

[Thomas Bertels]

I re-did the test and got the same result.
It works fine with version 1.3.

The addon id is jid0-iBZuKFBqP1RMtKM1cLEyUfrmAw8@jetpack

Be careful as it will make your Firefox restart in an endless loop. Before testing it, you should open the addon manager in a new tab to be able to disable the addon if you reproduce the bug.
You could also create a store.json with {"state":"idle"} inside to make it stop.

About the storage dir, the \jetpack\myname\simple-storage dir gets created, but nothing gets written inside.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Tomas, Sorry I'm still not able to reproduce this bug. I'm using exactly the same Id as you do `jid0-iBZuKFBqP1RMtKM1cLEyUfrmAw8@jetpack`. It creates a directory here:

<profile>/jetpack/jid0-iBZuKFBqP1RMtKM1cLEyUfrmAw8@jetpack/simple-storage/

I'm also able to strore data and read data after restarts.

Please note that file is not created immediately, it's saved in every 5 minutes or on shutdown. Which happens just fine as well.

Comment 5

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

(In reply to Thomas Bertels from comment #3)
> About the storage dir, the \jetpack\myname\simple-storage dir gets created,
> but nothing gets written inside.

We don't write to the file until the Firefox shuts down, to save on file I/O. So while Firefox runs the addon for the first time, the folder will be created for it, but the store.json file won't be there until you close Firefox or disable the addon.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

So I'm getting close to understanding to what issues is. So if you would create a new add-on now that has no `id` in the package.json `cfx run` will generate and `id` for you. Which before used to have `@jetpack` prefix which is no longer a case. That being said I don't understand how that is breaking a simple-storage module.

Also existing add-on's would have `id` in the package.json there for they will just continue to use the right place.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> Be careful as it will make your Firefox restart in an endless loop. Before testing > it, you should open the addon manager in a new tab to be able to disable the addon > if you reproduce the bug.

This suggests me that there might be something wrong with your add-on code or with your environment. If you have source of you're add-on somewhere online that would help a lot. Alternatively, I'd suggest reducing the test case, try to comment out all the code other then use of `simple-storage` and maybe also disable all other add-ons.

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I finally can reproduce an issue, working on it with warner.

Comment 9

[Dave Townsend [:mossop]]

Sounding like we might want to do another (sigh) hotfix for this

Comment 10

[Brian Warner [:warner :bwarner]]

Irakli and I tracked down most of this. What we know so far:

* commit e48f08b67f8bcfcbc3b869169e0c4e4559573fd8, part of Erik Vold's
  simple-prefs work (which landed on trunk in pull-request #270 on
  28-Nov-2011) included the following erroneous clause in packaging.py:

    if ('id' in target_cfg):
        build['jetpackID'] = target_cfg.id

* that clause happens to clobber the efforts of other code (in __init__.py)
  to put a .jetpackID value that has "@jetpack" appended. The result is that
  a package.json with "id: 'abc'" results in a harness-options.json with
  "jetpackID: 'abc@jetpack'" in 1.3, but "jetpackID: 'abc'" in 1.4
* simple-storage uses jetpackID to index its storage, so any changes to
  jetpackID will break persistence of simple-storage
* the build['jetpackID']= code was added to make unit-tests start passing.
  These tests (in test_xpi.py) use a helper function named create_xpi() that
  valiantly attempts to factor out the messy XPI-generation functionality
  from __init__.py . However, the messy-monster triumphed, because the code
  that is supposed to create .jetpackID (and happens to append @jetpack) is
  hopelessly buried inside __init__.py, but xpi.build_xpi() depends upon
  having a .jetpackID field. Adding .jetpackID in packaging.py probably
  looked like the best solution at that moment, and the subtly bad
  interaction with __init__.py would be really hard to spot.

So really this is a consequence of technical debt coming due. I should have
ripped __init__.py into cleaner pieces a year ago (bug 674778), which could
have made it possible to test XPI generation without such heroics. Meanwhile,
I'll look for a quick fix for this issue. The actual fix is easy (remove that
clause in packaging.py), but the trickier part is how to let the unit tests
pass without that bandage in place (and how to add a new unit test, to make
sure this problem is caught earlier next time).

Comment 11

[Brian Warner [:warner :bwarner]]

Attachment: fix jetpackID generation, add tests

This removes the code in packaging.py which accidentally override the code in
__init__.py which converts package.json:id into
harness-options.json:jetpackID . The consequence of that override was that a
"@jetpack" suffix was not appended when necessary, which caused "jetpackID"
to be wrong in 1.4, which caused simple-storage to look in the wrong place
for its saved data.

This also changes the build_xpi() convenience method (used by a couple tests)
to include the id-to-jetpackID conversion step. The lack of that conversion
step is what prompted the addition to packaging.py, as it was the quickest
way to get the tests to pass at the time.

The simple-prefs tests have been enhanced, and new tests were added to assert
that the id-to-jetpackID conversion happens properly for both "jid" and
"jid@jetpack" (i.e. with and without suffix-adding).

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 594058 [details] [diff] [review]
fix jetpackID generation, add tests

This code looks good and has tests - so while I'm not familiar with the code, rs=me :)

Comment 13

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/582a8ea90956a83ba823e1f2f77963bd30c1d8a1
Bug 723012: fix jetpackID which accidentally changed in 1.4 . r=mhammond

This removes the code in packaging.py which accidentally override the code in
__init__.py which converts package.json:id into
harness-options.json:jetpackID . The consequence of that override was that a
"@jetpack" suffix was not appended when necessary, which caused "jetpackID"
to be wrong in 1.4, which caused simple-storage to look in the wrong place
for its saved data.

This also changes the build_xpi() convenience method (used by a couple tests)
to include the id-to-jetpackID conversion step. The lack of that conversion
step is what prompted the addition to packaging.py, as it was the quickest
way to get the tests to pass at the time.

The simple-prefs tests have been enhanced, and new tests were added to assert
that the id-to-jetpackID conversion happens properly for both "jid" and
"jid@jetpack" (i.e. with and without suffix-adding).

Comment 14

[[github robot]]

Commit pushed to release at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/34d7cf81c6487c75b646d87fb727134325c8f118
Bug 723012: fix jetpackID which accidentally changed in 1.4 . r=mhammond

This removes the code in packaging.py which accidentally override the code in
__init__.py which converts package.json:id into
harness-options.json:jetpackID . The consequence of that override was that a
"@jetpack" suffix was not appended when necessary, which caused "jetpackID"
to be wrong in 1.4, which caused simple-storage to look in the wrong place
for its saved data.

This also changes the build_xpi() convenience method (used by a couple tests)
to include the id-to-jetpackID conversion step. The lack of that conversion
step is what prompted the addition to packaging.py, as it was the quickest
way to get the tests to pass at the time.

The simple-prefs tests have been enhanced, and new tests were added to assert
that the id-to-jetpackID conversion happens properly for both "jid" and
"jid@jetpack" (i.e. with and without suffix-adding).

Cherry-picked from master: 582a8ea and b1dbb63

Comment 15

[[github robot]]

Commits pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/34d7cf81c6487c75b646d87fb727134325c8f118
Bug 723012: fix jetpackID which accidentally changed in 1.4 . r=mhammond

https://github.com/mozilla/addon-sdk/commit/ffd4219c181908f950e4d96f5b6b11d3c4e7d544
Bug 723012: fix jetpackID which accidentally changed in 1.4 . r=mhammond

cherry-picked from 582a8ea90956a83ba823e1f2f77963bd30c1d8a1

This removes the code in packaging.py which accidentally override the code in
__init__.py which converts package.json:id into
harness-options.json:jetpackID . The consequence of that override was that a
"@jetpack" suffix was not appended when necessary, which caused "jetpackID"
to be wrong in 1.4, which caused simple-storage to look in the wrong place
for its saved data.

This also changes the build_xpi() convenience method (used by a couple tests)
to include the id-to-jetpackID conversion step. The lack of that conversion
step is what prompted the addition to packaging.py, as it was the quickest
way to get the tests to pass at the time.

The simple-prefs tests have been enhanced, and new tests were added to assert
that the id-to-jetpackID conversion happens properly for both "jid" and
"jid@jetpack" (i.e. with and without suffix-adding).

Comment 16

[[github robot]]

Commit pushed to release at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/ffd4219c181908f950e4d96f5b6b11d3c4e7d544
Bug 723012: fix jetpackID which accidentally changed in 1.4 . r=mhammond