1363760 - Install specialpowers as a non-temporary addon in tests

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Description

[Alex Gaynor [:Alex_Gaynor]]

Currently these are performed by the content process, which is causing challenges as we move to further restrict which files are readable. The most proximate challenge is that when running |mochitest| with a packaged build, the src directory where the SpecialPowers JS files are has no relationship to where the running binary is.

We should figure out where/why these are being loaded by content, and see if we can't move the file open/read into the parent.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Here's a pair of stack traces for trying to read one of these files:

            libsystem_kernel.dylib`lstat$INODE64+0xa
            XUL`nsFileChannel::nsFileChannel(nsIURI*)+0x22f
            XUL`nsFileProtocolHandler::NewChannel2(nsIURI*, nsILoadInfo*, nsIChannel**)+0x5e
            XUL`mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsILoadInfo*, nsIChannel**)+0x376
            XUL`NS_NewChannelInternal(nsIChannel**, nsIURI*, nsILoadInfo*, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*)+0xda
            XUL`nsChromeProtocolHandler::NewChannel2(nsIURI*, nsILoadInfo*, nsIChannel**)+0x311
            XUL`mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsILoadInfo*, nsIChannel**)+0x376
            XUL`mozilla::net::nsIOService::NewChannelFromURIWithProxyFlags2(nsIURI*, nsIURI*, unsigned int, nsIDOMNode*, nsIPrincipal*, nsIPrincipal*, unsigned int, unsigned int, nsIChannel**)+0x18c
            XUL`mozilla::net::nsIOService::NewChannelFromURI2(nsIURI*, nsIDOMNode*, nsIPrincipal*, nsIPrincipal*, unsigned int, unsigned int, nsIChannel**)+0x29
            XUL`NS_NewChannelInternal(nsIChannel**, nsIURI*, nsINode*, nsIPrincipal*, nsIPrincipal*, unsigned int, unsigned int, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*)+0x148
            XUL`NS_NewChannel(nsIChannel**, nsIURI*, nsIPrincipal*, unsigned int, unsigned int, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*)+0x30
            XUL`nsMessageManagerScriptExecutor::TryCacheLoadAndCompileScript(nsAString const&, bool, bool, JS::MutableHandle<JSScript*>)+0x386
            XUL`nsMessageManagerScriptExecutor::LoadScriptInternal(nsAString const&, bool)+0xc4
            XUL`non-virtual thunk to mozilla::dom::TabChild::RecvLoadRemoteScript(nsString const&, bool const&)+0x3a
            XUL`mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&)+0x852c
            XUL`mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&)+0x4da
            XUL`mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)+0x159
            XUL`mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&)+0x2c5
            XUL`mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&)+0x315
            XUL`mozilla::ipc::MessageChannel::MessageTask::Run()+0x7c


and


            libsystem_kernel.dylib`stat$INODE64+0xa
            XUL`nsFileChannel::MakeFileInputStream(nsIFile*, nsCOMPtr<nsIInputStream>&, nsCString&, bool)+0x40
            XUL`nsFileChannel::OpenContentStream(bool, nsIInputStream**, nsIChannel**)+0x530
            XUL`nsBaseChannel::Open(nsIInputStream**)+0x69
            XUL`nsBaseChannel::Open2(nsIInputStream**)+0x4d
            XUL`nsMessageManagerScriptExecutor::TryCacheLoadAndCompileScript(nsAString const&, bool, bool, JS::MutableHandle<JSScript*>)+0x3e7
            XUL`nsMessageManagerScriptExecutor::LoadScriptInternal(nsAString const&, bool)+0xc4
            XUL`non-virtual thunk to mozilla::dom::TabChild::RecvLoadRemoteScript(nsString const&, bool const&)+0x3a
            XUL`mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&)+0x852c
            XUL`mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&)+0x4da
            XUL`mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)+0x159
            XUL`mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&)+0x2c5
            XUL`mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&)+0x315
            XUL`mozilla::ipc::MessageChannel::MessageTask::Run()+0x7c
            XUL`mozilla::SchedulerGroup::Runnable::Run()+0xb5
            XUL`nsThread::ProcessNextEvent(bool, bool*)+0x5db
            XUL`NS_ProcessNextEvent(nsIThread*, bool)+0x50
            XUL`mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+0x1a3
            XUL`MessageLoop::Run()+0x4a
            XUL`XRE_RunAppShell()+0x105

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Something that might work here: have the script in the parent process create a File object[1], then register a blob URL[2] and send that to the content process.

[1] https://developer.mozilla.org/en-US/docs/Extensions/Using_the_DOM_File_API_in_chrome_code
[2] https://developer.mozilla.org/en-US/docs/Web/API/URL/createObjectURL — as of bug 1279186 this should broadcast a PBlob to all processes

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

If I've read the code correctly, the offender is one of:

- https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowers.js#215-217
- https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/SpecialPowersObserver.jsm#91-93

Someone on IRC also suggested that data URIs may be a solution.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

ni? Andy for the addons perspective and Joel for the mochitest perspective. Hopefully this message has enough context to explain why.

After some more research and discussion with the rest of the sandboxing team, I'm going to attempt to summarize where we're at:

When running mochitest (and reftest, it looks like), `specialpowers` is installed as a temporary addon. Then in the parent process we tell the message manager to load a frame script in our tab, with a `chrome://specialpowers/.../MozillaLogger.js` URL (and a few others).

Digging into how this is implemented:

- When installing a temporary addon, it appears that the code doesn't copy the addon contents anywhere, instead it just wires up some metadata to the existing disk location
- Loading a chrome:// URL basically rewrites it as a file:// URL and tries to load it in the content process.

As we ratchet up read restrictions for the sandbox, this will break mochitest with packaged builds, but I believe it'll break some (all?) uses of install temporary legacy addons.

This leaves us with a few questions:

1) Are the specialpowers and legacy addons issues going to be the same, or have I misread one or both of how they're setup?
2) Is it necessary to continue to support temporary installation of legacy addons, give they're going away entirely in 6 months
3) What's the right way to fix this?

For web-extensions we have bug 1334550 to proxy all their loads to the parent process. Is the fix here to install specialpowers to an existing whitelisted directory? To implement something specific to it? To package it as a web-extension? (No idea if this makes sense :-)). Something else?

Appreciate any insight into how we can make everybody happy here!

Comment 5

[Andrew Swan [:aswan]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #4)
> Digging into how this is implemented:
> 
> - When installing a temporary addon, it appears that the code doesn't copy
> the addon contents anywhere, instead it just wires up some metadata to the
> existing disk location
> - Loading a chrome:// URL basically rewrites it as a file:// URL and tries
> to load it in the content process.

I don't think this is specific to temporary installation, permanent installation works the same way.
Are you sure it gets rewritten to file: ?  I would expect it to be jar:  That doesn't really matter though, jar has the same problem.

> As we ratchet up read restrictions for the sandbox, this will break
> mochitest with packaged builds, but I believe it'll break some (all?) uses
> of install temporary legacy addons.

Right, to be precise this will break any legacy extensions that register chrome: resources.

> This leaves us with a few questions:
> 
> 1) Are the specialpowers and legacy addons issues going to be the same, or
> have I misread one or both of how they're setup?

Not sure I understand the question, are you asking if these addons are going to keep working the way they do right now?
I think your understanding of how they currently work is correct.  And I don't know of any plans to change how they work (other than this bug of course!)

> 2) Is it necessary to continue to support temporary installation of legacy
> addons, give they're going away entirely in 6 months

Legacy extensions signed by AMO won't be usable on release builds but we still use them heavily in automation (I don't have an exhaustive list handy but specialpowers, mochitest, reftest, talos, etc. all rely on legacy extensions that simply can't be written as webextensions) and we'll have some exceptions for things like shield and test pilot.

> 3) What's the right way to fix this?

I'm not sure.  How are other chrome: resources that end up mapping to files on disk handled in the content process?

Comment 6

[Haik Aftandilian [:haik]]

(In reply to Andrew Swan [:aswan] from comment #5)
> (In reply to Alex Gaynor [:Alex_Gaynor] from comment #4)
> > Digging into how this is implemented:
> > 
> > - When installing a temporary addon, it appears that the code doesn't copy
> > the addon contents anywhere, instead it just wires up some metadata to the
> > existing disk location
> > - Loading a chrome:// URL basically rewrites it as a file:// URL and tries
> > to load it in the content process.
> 
> I don't think this is specific to temporary installation, permanent
> installation works the same way.
> Are you sure it gets rewritten to file: ?  I would expect it to be jar: 
> That doesn't really matter though, jar has the same problem.

The difference for us is that a temporary installation uses the add-on files (or JAR file) where they are, but a permanent installation of an add-on copies the files to $PROFILE/extensions.

What we're trying to rollout is a sandbox that prevents the content process from reading the home directory (where sensitive data is stored) and other locations we know content doesn't need to read. We will have sandbox exception rules in place to allow the content process to access files in $PROFILE/extensions directly (to support legacy add-ons.)

Hence add-on files installed in $PROFILE/extensions can be read by content, but a temporary add-on, which can be installed from anywhere on the filesystem (most likely from the home directory), would run into file access failures.

> > As we ratchet up read restrictions for the sandbox, this will break
> > mochitest with packaged builds, but I believe it'll break some (all?) uses
> > of install temporary legacy addons.
> 
> Right, to be precise this will break any legacy extensions that register
> chrome: resources.

Not as long as we allow content to read from $PROFILE/extensions.

Legacy add-ons would be affected if they rely on loading files from the content process outside of $PROFILE/extensions. That's covered on https://blog.mozilla.org/addons/2017/02/16/the-road-to-firefox-57-compatibility-milestones/ We are not expecting that to be a large number of add-ons, but haven't got to testing this heavily yet.

We want to address mochitests on packaged builds too, but those use different directories we might be able to exclude for the time being.

And, to reiterate, this refers to legacy add-ons. For WebExtensions, we are working on remoting the moz-extension protocol which allows temporary and permanent installs of WebExtensions to work without special sandbox rules. That's bug 1334550.

Comment 7

[Andrew Swan [:aswan]]

(In reply to Haik Aftandilian [:haik] from comment #6)
> (In reply to Andrew Swan [:aswan] from comment #5)
> > (In reply to Alex Gaynor [:Alex_Gaynor] from comment #4)
> > > Digging into how this is implemented:
> > > 
> > > - When installing a temporary addon, it appears that the code doesn't copy
> > > the addon contents anywhere, instead it just wires up some metadata to the
> > > existing disk location
> > > - Loading a chrome:// URL basically rewrites it as a file:// URL and tries
> > > to load it in the content process.
> > 
> > I don't think this is specific to temporary installation, permanent
> > installation works the same way.
> > Are you sure it gets rewritten to file: ?  I would expect it to be jar: 
> > That doesn't really matter though, jar has the same problem.
> 
> The difference for us is that a temporary installation uses the add-on files
> (or JAR file) where they are, but a permanent installation of an add-on
> copies the files to $PROFILE/extensions.

Ah, I had assumed specialpowers got built into an xpi and of course the exception for {profile}/extensions does make a big difference.

If that exception is going to live on for legacy extensions, then I think we're actually in good shape here -- when bug 1356826 lands we can switch from temporary to permanent installation and everything should work.

I suspect there are a bunch of mochitests that use specialpowers to get at nsIFile/FileUtils/whatever and hit the filesystem which will break, but that's a separate issue.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

1) Are the specialpowers and legacy addons issues going to be the same, or have I misread one or both of how they're setup?
you have read this correctly- that is how we setup addons

2) Is it necessary to continue to support temporary installation of legacy addons, give they're going away entirely in 6 months
my understanding is there is no work being done to change in-tree test related addons (we have 30+).  They are all legacy.  For Talos we sign our addons, for reftest/mochitest we install temporarily via marionette.  I have asked dozens of times if we need to update our test addons and I am always told it will be fine (even just a couple weeks ago).  My understanding is that using the pref "security.turn_off_all_security_so_that_viruses_can_take_over_this_computer" we will be fine.  Should we be planning on converting the test addons in all cases to web extensions before Firefox 57 ships?  If so we will need additional help to do this.

3) What's the right way to fix this?
this I don't know.

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Echoing back what I read:

- We'll continue having legacy addons for testing purposes well past 57. (As an aside, I hadn't seen turn_off_all_security_so_that_viruses_can_take_over_this_computer before, and when you mentioned it I thought it was a joke at first :-))

- Once bug 1356826 lands we can change installing specialpowers to just be a permanent addon, copying it to $PROFILE/extensions, and this will fix the current pain point. This is already on the addons team's radar, so yay! :-)

- We do have a ton of other testing addons, so there may be others that'll need similar treatment.

Thanks for the insights Joel and Andrew, very much appreciated. It seems like we're in a good place!

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Changed the bug title to accurately reflect what the real solution will be.

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

This means they will be copied to $PROFILE/extensions which the sandbox allows
access to; if they are installed as temporary addons, loading frame scripts in
the content process tries to read from wherever they happen to be on disk. This
breaks running tests with a packaged build once we have full read-restrictions
for the content process sandbox.

r?aswan

Review commit: https://reviewboard.mozilla.org/r/139302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139302/

Comment 12

[Haik Aftandilian [:haik]]

Is there anything we have to do here to make sure test addons are cleanly uninstalled (after a crash)? If that even matters given that we use temp profiles.

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139302/diff/1-2/

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

https://reviewboard.mozilla.org/r/139302/#review142660

::: testing/mochitest/runtests.py:2137
(Diff revision 2)
>  
>              # install specialpowers and mochikit as temporary addons
>              addons = Addons(self.marionette)
>  
>              if mozinfo.info.get('toolkit') != 'gonk':
> -                addons.install(os.path.join(here, 'extensions', 'specialpowers'), temp=True)
> +                addons.install(create_zip(

I'm not up on all the details here but I see specialpowers already packaged as an xpi in OBJDIR/dist/xpi-stage/[e-mail]
Can you just copy that over?  And do the same for mochijar?

Comment 15

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

https://reviewboard.mozilla.org/r/139302/#review142660

> I'm not up on all the details here but I see specialpowers already packaged as an xpi in OBJDIR/dist/xpi-stage/[e-mail]
> Can you just copy that over?  And do the same for mochijar?

Will that work for packaged builds? Do we have an `$OBJDIR` with things in it on the machine running tests, or just the source?

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

https://reviewboard.mozilla.org/r/139302/#review142660

> Will that work for packaged builds? Do we have an `$OBJDIR` with things in it on the machine running tests, or just the source?

If `here` in that code is not an OBJDIR, what is it?

Comment 17

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

https://reviewboard.mozilla.org/r/139302/#review142660

> If `here` in that code is not an OBJDIR, what is it?

`here = os.path.abspath(os.path.dirname(__file__))`

Comment 18

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

https://reviewboard.mozilla.org/r/139302/#review142660

> `here = os.path.abspath(os.path.dirname(__file__))`

Mmmm, no you're right, `here` is inside `$OBJDIR` - I'll try the change you've proposed.

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

While using the pre-built XPI appears to work well enough (at least locally), I'm working through some test failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3e392cb0a0e&group_state=expanded

From debugging, it looks like the problem is that in `uninstallAddon`, the `yield addonListMutation` never fires -- nothing in the `getAddonListWithAddon` or `waitForMutation` looks like it should suffer from one other addon happening to be installed, but I'm still in the process of debugging.

Comment 20

[Alex Gaynor [:Alex_Gaynor]]

Debugging notes: Using a non-temporary specialpowers with a temporary mochijar does not exhibit the same bug.

Upon further investigation the problem appears to be that the DOM queries that are performed to test that things were properly uninstalled do not handle going from 1 => 0 temporary addons, which lots of these tests do once mochijar and specialpowers are not temporary.

Specific problems:

(1) `waitForMutation(node, { childList: true })` does not consider a mutation to have occurred if `node` is removed from the DOM (teehee). This hits us at https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/head.js?q=function%2A+uninstallAddon&redirect_type=single#215-236
(2) Further, given a `node`, if that node is removed from the DOM, `node.querySelectorAll("...")` will happily return some of it's children nodes even though none of it is attached to `document` anymore, whereas if a child of `node` is removed, `node.querySelectorAll("...")` obviously doesn't return in it. This hits us at https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/head.js?q=function%2A+uninstallAddon&redirect_type=single#237-241

The root of these is that when we've got >1 temporary addon installed and we remove it, we delete an <li> from the <ul> we're observing. When we're removing the final temporary addon, we delete the whole <ul> (and stick a `<p>Nothing yet.</p>` in its place).

There may be other issues, I've only debugged one particular failure, but these seem very likely to be a pattern which recurs, so it wouldn't surprise me if these accounted for all the failures.

It's been a while since I DOM'd in earnest, so feedback on this would be appreciated.

Issue (1) can be resolved be using `waitForMutation(addonList.parentNode, { childList: true, subtree: true })` which should work no matter how many addons are installed.

Issue (2) I'm less certain about, what's the right way to express "children which match this query selector, but only if I'm still attached to document".

Comment 21

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139302/diff/2-3/

Comment 22

[Alex Gaynor [:Alex_Gaynor]]

> XPI file /builds/slave/test/build/tests/mochitest/../../../dist/xpi-stage/[e-mail] does not exist

Seems to indicate that the XPIs aren't around in automation, once we get to running tests.

Comment 23

[Andrew Swan [:aswan]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

https://reviewboard.mozilla.org/r/139302/#review143590

If you want sign-off on the concept of using regular (non-temporary) installation, r=me but a test/build peer should sign off on the actual implementation.

Comment 24

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139302/diff/3-4/

Comment 25

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139302/diff/4-5/

Comment 26

[Andrew Halberstadt [:ahal]]

I'm the one who started installing these temporarily in the first place back when addon signing was rolling out, so would be happy to do reviews on the mochitest side of things.

Comment 27

[Alex Gaynor [:Alex_Gaynor]]

:ahal, awesome, thanks! Just as soon as I've got the try bots happy I'll r? you. (Several tests for about:addons make assumptions about the shape of the DOM)

Comment 28

[Andrew Halberstadt [:ahal]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

https://reviewboard.mozilla.org/r/139302/#review147636

Thanks for looking into this! I don't really understand the /devtools and /toolkit changes, it might be a good idea to split those into a separate commit and get someone more relevant to review them. As far as the mochitest harness, looks good, nice and simple! I'll change to an r+ with the issue fixed.

::: testing/mochitest/runtests.py:779
(Diff revision 5)
> +def _add_to_zip(z, path, zippath):
> +    """
> +    Adds a `path` on disk to the zip `z` at location `zippath` within in.
> +    """
> +    if os.path.isfile(path):
> +        z.write(path, zippath, zipfile.ZIP_DEFLATED)
> +    elif os.path.isdir(path):
> +        if zippath:
> +            z.write(path, zippath)
> +        for name in os.listdir(path):
> +            _add_to_zip(z, os.path.join(path, name), os.path.join(zippath, name))
> +
> +
> +def create_zip(path):
> +    """
> +    Takes a `path` on disk and creates a zipfile with its contents. Returns a
> +    path to the location of the temporary zip file.
> +    """
> +    with tempfile.NamedTemporaryFile(delete=False) as f:
> +        with zipfile.ZipFile(f, "w") as z:
> +            _add_to_zip(z, path, "")
> +    return f.name

There's a higher level utility for this in the stdlib, under `shutil.make_archive`. I believe you can use it like:

    shutil.make_archive(<filename>, 'zip', <directory>)

Comment 29

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1363760 - Part 1 - Allow the about:debugging tests to pass if there are no temporary addons installed

The current DOM tests for about:debugging implicitly assume that there is a
temporary addon installed before they start. This is currently always true
because mochitest and specialpowers are installed as temporary addons, but the
goal of this bug is to install them as non-temporary addons.

The specific cause of the breakage is the assumption that uninstalling an addon
removes an <li> from the addons <ul>, but when you go from one addon to zero the
entire <ul> is removed.

Review commit: https://reviewboard.mozilla.org/r/144166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144166/

Comment 30

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1363760 - Part 2 - Corrected the permission that is reset at the end of a test

Without this change, browser_update.js "resets" a preference that it never
changed to a different value, which leaks through to future tests. This was
introduced in a8fcca075fde, and appears to be a simple mistake since that change
removes a setup/teardown pref change pair, but the prefs it changes are two
different ones!

This leaked pref change leads to test failures when special powers and mochitest
are installed as non-temporary addons.

Review commit: https://reviewboard.mozilla.org/r/144168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144168/

Comment 31

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139302/diff/5-6/

Comment 32

[Andrew Swan [:aswan]]

Comment on attachment 8872632 [details]
Bug 1363760 - Part 2 - Corrected the permission that is reset at the end of a test

https://reviewboard.mozilla.org/r/144168/#review147902

This is an old test that was written before `SpecialPowers.pushPrefEnv()` existed, can you just change the call to `Services.prefs.setBoolPref()` to use that instead, then there's no need to clean up.

Comment 33

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8872632 [details]
Bug 1363760 - Part 2 - Corrected the permission that is reset at the end of a test

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144168/diff/1-2/

Comment 34

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139302/diff/6-7/

Comment 35

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8872632 [details]
Bug 1363760 - Part 2 - Corrected the permission that is reset at the end of a test

https://reviewboard.mozilla.org/r/144168/#review147902

Done.

Comment 36

[Alex Gaynor [:Alex_Gaynor]]

Not sure what I'm doing wrong, but bz and mozreview seem to be enjoying getting out of sync. I've attempted to make sure they're both in agreement, since I have no idea what actual notifications people get otherwise.

Comment 37

[Andrew Swan [:aswan]]

Comment on attachment 8872632 [details]
Bug 1363760 - Part 2 - Corrected the permission that is reset at the end of a test

https://reviewboard.mozilla.org/r/144168/#review147926

thanks!

Comment 38

[Andrew Halberstadt [:ahal]]

Comment on attachment 8867757 [details]
Bug 1363760 - Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons

https://reviewboard.mozilla.org/r/139302/#review147984

Thanks, lgtm!

Comment 39

[Julian Descottes [:jdescottes]]

Comment on attachment 8872631 [details]
Bug 1363760 - Part 1 - Allow the about:debugging tests to pass if there are no temporary addons installed

https://reviewboard.mozilla.org/r/144166/#review148704

LGTM, thanks!

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65fcedb6c1f8
Part 1 - Allow the about:debugging tests to pass if there are no temporary addons installed r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0205cdd17a67
Part 2 - Corrected the permission that is reset at the end of a test r=aswan
https://hg.mozilla.org/integration/autoland/rev/71e0ff5a6c76
Part 3 - In tests, install SpecialPowers and mochijar as non-temporary addons r=ahal

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65fcedb6c1f8
https://hg.mozilla.org/mozilla-central/rev/0205cdd17a67
https://hg.mozilla.org/mozilla-central/rev/71e0ff5a6c76