Install specialpowers as a non-temporary addon in tests

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: sbmc3)

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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
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
(Assignee)

Comment 4

2 years ago
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!
Flags: needinfo?(jmaher)
Flags: needinfo?(amckay)

Updated

2 years ago
Flags: needinfo?(amckay) → needinfo?(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?
Flags: needinfo?(aswan)
(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.
(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.
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.
Flags: needinfo?(jmaher)
(Assignee)

Comment 9

2 years ago
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!
(Assignee)

Updated

2 years ago
Blocks: 1357758
Depends on: 1356826

Updated

2 years ago
Whiteboard: sbmc3
(Assignee)

Updated

2 years ago
Summary: Move file reads of special-powers javascript to parent process → Install specialpowers as a non-temporary addon in tests
(Assignee)

Comment 10

2 years ago
Changed the bug title to accurately reflect what the real solution will be.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8867757 - Flags: review?(aswan)
(Assignee)

Updated

2 years ago
Assignee: nobody → agaynor
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 hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
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/specialpowers@mozilla.org.xpi
Can you just copy that over?  And do the same for mochijar?
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
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/specialpowers@mozilla.org.xpi
> 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

2 years ago
mozreview-review-reply
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?
(Assignee)

Comment 17

2 years ago
mozreview-review-reply
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__))`
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 19

2 years ago
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.
(Assignee)

Comment 20

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 22

2 years ago
> XPI file /builds/slave/test/build/tests/mochitest/../../../dist/xpi-stage/specialpowers@mozilla.org.xpi does not exist

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

Comment 23

2 years ago
mozreview-review
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.
Attachment #8867757 - Flags: review?(aswan)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
(Assignee)

Comment 27

2 years ago
: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)
(Assignee)

Updated

2 years ago
Attachment #8867757 - Flags: review?(aswan) → review?(ahalberstadt)

Comment 28

2 years ago
mozreview-review
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>)
Attachment #8867757 - Flags: review?(ahalberstadt) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8872631 - Flags: review?(jdescottes)
Attachment #8872632 - Flags: review?(amckay)
Attachment #8867757 - Flags: review?(ahalberstadt)
(Assignee)

Updated

2 years ago
Attachment #8867757 - Flags: review?(ahalberstadt)

Updated

2 years ago
Attachment #8872632 - Flags: review?(amckay) → review?(aswan)

Comment 32

2 years ago
mozreview-review
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.
Attachment #8872632 - Flags: review?(aswan)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 years ago
mozreview-review-reply
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.
(Assignee)

Updated

2 years ago
Attachment #8872632 - Flags: review?(amckay)
Attachment #8867757 - Flags: review?(ahalberstadt)
(Assignee)

Updated

2 years ago
Attachment #8872632 - Flags: review?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8867757 - Flags: review?(ahalberstadt)
(Assignee)

Comment 36

2 years ago
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

2 years ago
mozreview-review
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!
Attachment #8872632 - Flags: review?(aswan) → review+

Comment 38

2 years ago
mozreview-review
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!
Attachment #8867757 - Flags: review?(ahalberstadt) → review+

Comment 39

2 years ago
mozreview-review
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!
Attachment #8872631 - Flags: review?(jdescottes) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 40

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 41

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/65fcedb6c1f8
https://hg.mozilla.org/mozilla-central/rev/0205cdd17a67
https://hg.mozilla.org/mozilla-central/rev/71e0ff5a6c76
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.