825471 - Jetpack broken due to changes to the old private browsing service

Status: RESOLVED FIXED

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

Description

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

Looks like there's an exception thrown during the example package "reading-data"'s tests about the private browsing service being undefined on recent inbound and mozilla-central test runs, and the test harness then times out soon after:

Error: TypeError: Cc['@mozilla.org/privatebrowsing;1'] is undefined 
 Traceback (most recent call last):
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/private-browsing/utils.js", line 24, in 
    pbService = Cc["@mozilla.org/privatebrowsing;1"].
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/windows/dom.js", line 8, in 
    const { getMode } = require('../private-browsing/utils');
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/windows/firefox.js", line 11, in 
    { WindowDom } = require('./dom'),
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/windows.js", line 11, in 
    module.exports = require('./windows/firefox');
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/widget.js", line 48, in 
    const windowsAPI = require("./windows");
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/reading-data/lib/main.js", line 7, in 
    var widgets = require("addon-kit/widget");
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/reading-data/tests/test-main.js", line 5, in 
    var m = require("main");
0 of 1 tests passed.
error: reading-data: An exception occurred.
TypeError: tests.testRuns is undefined
resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/runner.js 57
Traceback (most recent call last):
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/timers.js", line 31, in notify
    callback.apply(null, args);
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/runner.js", line 47, in 
    onDone: onDone
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/harness.js", line 335, in runTests
    onDone({passed: 0, failed: 1});
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/runner.js", line 29, in onDone
    printFailedTests(tests, cfxArgs.verbose, stdout.write);
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/runner.js", line 57, in printFailedTests
    let singleIteration = tests.testRuns.length == 1;
console: [JavaScript Error: "reading-data: An exception occurred.
TypeError: tests.testRuns is undefined
resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/runner.js 57
Traceback (most recent call last):
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/timers.js", line 31, in notify
    callback.apply(null, args);
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/runner.js", line 47, in 
    onDone: onDone
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/harness.js", line 335, in runTests
    onDone({passed: 0, failed: 1});
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/runner.js", line 29, in onDone
    printFailedTests(tests, cfxArgs.verbose, stdout.write);
  File "resource://reading-data-example-at-jetpack-dot-mozillalabs-dot-com/addon-sdk/lib/sdk/test/runner.js", line 57, in printFailedTests
    let singleIteration = tests.testRuns.length == 1;
"]
Traceback (most recent call last):
  File "bin/cfx", line 33, in <module>
    cuddlefish.run()
  File "/home/cltbld/talos-slave/test/build/addon-sdk-3fcfb7b6e03d/python-lib/cuddlefish/__init__.py", line 574, in run
    test_all(env_root, defaults=options.__dict__)
  File "/home/cltbld/talos-slave/test/build/addon-sdk-3fcfb7b6e03d/python-lib/cuddlefish/__init__.py", line 343, in test_all
    test_all_examples(env_root, defaults)
  File "/home/cltbld/talos-slave/test/build/addon-sdk-3fcfb7b6e03d/python-lib/cuddlefish/__init__.py", line 421, in test_all_examples
    env_root=env_root)
  File "/home/cltbld/talos-slave/test/build/addon-sdk-3fcfb7b6e03d/python-lib/cuddlefish/__init__.py", line 858, in run
    mobile_app_name=options.mobile_app_name)
  File "/home/cltbld/talos-slave/test/build/addon-sdk-3fcfb7b6e03d/python-lib/cuddlefish/runner.py", line 675, in run_app
    OUTPUT_TIMEOUT)
Exception: Test output exceeded timeout (60s).
program finished with exit code 1
elapsedTime=99.805609
TinderboxPrint:jetpack<br/>3/1
Unknown Error: command finished with exit code: 1






I'm not sure yet if things are really truly broken or if it's just the test harness that's broken. Will investigate soon.

Comment 1

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

Attachment: Catch failures when accessing privateBrowsingService

So, with this patch applied, the tests run on the latest build from mozilla-central, which includes the removal of the global private browsing service.

It effectively disables our private-browsing tests in test/test-private-browsing.js, but they all look like they're only using the global service, so they wouldn't pass anyway.


Oddly, cfx testall with this applied says that 5880 of 5880 tests passed, but that some tests were unsuccessful. It never says where it was unsuccessful, though. :\

Comment 2

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

Oh, and just for fun, I tried installing one of my addons (without the attached patch from this bug) into a profile running on a tinderbox build with bug 818800 included.

My addon doesn't directly use the private-browsing module itself, but it still threw an error about the privatebrowsingservice being undefined upon install, and failed to work.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Oh, and just for fun, I tried installing one of my addons (without the attached
> patch from this bug) into a profile running on a tinderbox build with bug
> 818800 included.
> 
> My addon doesn't directly use the private-browsing module itself, but it still
> threw an error about the privatebrowsingservice being undefined upon install,
> and failed to work.

See bug 748604.

Comment 4

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

So, if this is breaking actual addons (even if they don't use the private-browsing module themselves), that means we're gonna need another mass repack, right, Jeff?

Comment 5

[Dave Townsend [:mossop]]

Looks like this will break any add-ons using the tabs or windows modules and packed with SDK 1.11 or higher.

Comment 6

[(no longer active)]

Sadness. :(  Does comment 4 sound like the correct way to address this?

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

Sucks.  I should've seen the scope of issue with removing the service sooner.  Eshan is it possible to merely deprecate the pb service for awhile?  I think this will be important for many non SDK based addons as well.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Ehsan Akhgari [:ehsan] (Mostly away until Jan 2) from comment #6)
> Sadness. :(  Does comment 4 sound like the correct way to address this?

Re-packs don't generally get us 100% coverage and take time to roll out, it would be much better to leave the pb service in place in the platform and just have it always say it isn't in pb mode or something.

Comment 9

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

Landed: https://github.com/mozilla/addon-sdk/commit/7882a363816bb4acd82d75c01de91c558f47d1b1

Lets see how this goes...

Comment 10

[(no longer active)]

Gavin tells me that re-packs are a manual and painful process.  This is very disappointing since any code which uses this service is broken code which needs to get fixed.  I filed bug 826037 to restore a dummy service which implements an empty nsIPrivateBrowsingService, but I will be removing that service from Firefox 21, and I hope that this gives the Add-on SDK team enough time to prepare for a re-pack which fixes this code in all of the existing extensions.  *Please* let me know if that doesn't buy us enough time.  Thanks.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Ehsan Akhgari [:ehsan] (Mostly away until Jan 2) from comment #10)
> Gavin tells me that re-packs are a manual and painful process.  This is very
> disappointing since any code which uses this service is broken code which
> needs to get fixed.  I filed bug 826037 to restore a dummy service which
> implements an empty nsIPrivateBrowsingService, but I will be removing that
> service from Firefox 21, and I hope that this gives the Add-on SDK team
> enough time to prepare for a re-pack which fixes this code in all of the
> existing extensions.  *Please* let me know if that doesn't buy us enough
> time.  Thanks.

That about lines us up with our plans to land in Firefox and try to do repacks for that so that should work out well, thanks.

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Ehsan Akhgari [:ehsan] (Mostly away until Jan 2) from comment #10)
> Gavin tells me that re-packs are a manual and painful process.  This is very
> disappointing since any code which uses this service is broken code which
> needs to get fixed.  I filed bug 826037 to restore a dummy service which
> implements an empty nsIPrivateBrowsingService, but I will be removing that
> service from Firefox 21, and I hope that this gives the Add-on SDK team
> enough time to prepare for a re-pack which fixes this code in all of the
> existing extensions.  *Please* let me know if that doesn't buy us enough
> time.  Thanks.

Ehsan: thanks for putting in the dummy nsIPrivateBrowsingService service, this will allow us enough time to move on a few plans we have around add-on compatibility:

1. we've identified a bunch of add-ons that use really really old versions of the SDK, and we're going to mark them incompatible with Firefox 18.

2. we're looking at re-packing as many add-ons as possible to SDK 1.14, which will be the version of SDK that is included in Firefox 21. SDK 1.14 is due to be released on March 12th.

3. eventually, we may look at re-packing add-ons to no longer include the SDK's apis.

The real danger around this is the case where a very popular add-on hasn't been updated and can't be auto-re-packed. I need to do more investigation into the AMO data as we go to identify cases like that.

Comment 13

[(no longer active)]

(In reply to Jeff Griffiths (:canuckistani) from comment #12)
> (In reply to Ehsan Akhgari [:ehsan] (Mostly away until Jan 2) from comment
> #10)
> > Gavin tells me that re-packs are a manual and painful process.  This is very
> > disappointing since any code which uses this service is broken code which
> > needs to get fixed.  I filed bug 826037 to restore a dummy service which
> > implements an empty nsIPrivateBrowsingService, but I will be removing that
> > service from Firefox 21, and I hope that this gives the Add-on SDK team
> > enough time to prepare for a re-pack which fixes this code in all of the
> > existing extensions.  *Please* let me know if that doesn't buy us enough
> > time.  Thanks.
> 
> Ehsan: thanks for putting in the dummy nsIPrivateBrowsingService service,
> this will allow us enough time to move on a few plans we have around add-on
> compatibility:

Not a problem!  I just landed the patch to bug 826037 on inbound -- I'd appreciate if you guys can give it a shot.  In my local testing, it does fix the problem for extensions that don't use the private-browsing module.

> 1. we've identified a bunch of add-ons that use really really old versions
> of the SDK, and we're going to mark them incompatible with Firefox 18.
> 
> 2. we're looking at re-packing as many add-ons as possible to SDK 1.14,
> which will be the version of SDK that is included in Firefox 21. SDK 1.14 is
> due to be released on March 12th.

That's great timing!

> 3. eventually, we may look at re-packing add-ons to no longer include the
> SDK's apis.
> 
> The real danger around this is the case where a very popular add-on hasn't
> been updated and can't be auto-re-packed. I need to do more investigation
> into the AMO data as we go to identify cases like that.

Makes sense.  Please keep me and Josh in the loop.  Thanks!

Comment 15

[Alex Keybl [:akeybl]]

(In reply to Jeff Griffiths (:canuckistani) from comment #12)
> The real danger around this is the case where a very popular add-on hasn't
> been updated and can't be auto-re-packed. I need to do more investigation
> into the AMO data as we go to identify cases like that.

What about Jetpack add-ons not hosted on AMO? Perhaps that's not very prevalent. Can you get a bug on file (and tracked for FF21) so that we can follow your progress? The sooner we know, the better, given Ehsan's plans to rip out the service again in FF21.

Comment 16

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Alex Keybl [:akeybl] from comment #15)

Sure: https://bugzilla.mozilla.org/show_bug.cgi?id=827453

I don't think the best approach is to use Firefox 21 as an ultimatum.

Comment 17

[(no longer active)]

(In reply to comment #16)
> (In reply to Alex Keybl [:akeybl] from comment #15)
> 
> Sure: https://bugzilla.mozilla.org/show_bug.cgi?id=827453
> 
> I don't think the best approach is to use Firefox 21 as an ultimatum.

It is not an ultimatum, but we _do_ need to remove the service in order to catch all of the broken code inside non-Jetpack add-ons which use the PB service.  In fact I'm very uncomfortable that we had to reintroduce the broken service.

Comment 18

[Jeff Griffiths (:canuckistani) (:⚡︎)]

It's really about which large list of add-ons you think you're going to break and when. :)

Comment 19

[(no longer active)]

(In reply to comment #18)
> It's really about which large list of add-ons you think you're going to break
> and when. :)

Well we're also looking at other things, such as bug 826079.

As I've mentioned before, keeping the global service is not an option.  With changes to Firefox, sometimes we _have_ to break old APIs.  It's just a fact of life.

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/718

Pointer to Github pull-request

Comment 21

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

I'd like to cherry-pick this to stabilization by the end of the week (6 weeks earlier for developers to update addons to use the fixed private-browsing stuff), so hopefully it gets reviewed and landed on master (and nothing horribly broken) in the next few days.

Comment 22

[Jeff Griffiths (:canuckistani) (:⚡︎)]

+1 on the cherry-pick!

Comment 23

[(no longer active)]

Note that I did some drop-by code review here: https://github.com/mozilla/addon-sdk/pull/718

Comment 24

[Erik Vold [:erikvold] (please needinfo? me)]

I'm setting the TM to 1.13 for this, maybe we can up it to 1.14 though?

Comment 25

[Erik Vold [:erikvold] (please needinfo? me)]

The tests work again, the rest can be tracked on bug 748604