1512436 - Add new install location for built in search web extensions

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P1)

Comment 1

[Florian Quèze [:florian]]

Are we sure that this won't add any startup I/O before first paint?

Comment 2

[Dale Harvey (:daleharvey)]

We talked about performance worries. Andrew said the process shouldnt be expensive. 

We are going to be reading a bunch of json files from the omni jar instead of those xml files directly from disk afaik so it could be slower or faster, but will measure and if it causes any slowdown there are things we can do to make sure it is fast (like build the nssearchservice cache at build time for instance)

Any pointers to tests I could use to verify / measure these issues would be very useful, thanks

Comment 3

[Florian Quèze [:florian]]

(In reply to Dale Harvey (:daleharvey) from comment #2)
> We talked about performance worries. Andrew said the process shouldnt be
> expensive. 
> 
> We are going to be reading a bunch of json files from the omni jar instead
> of those xml files directly from disk

The xml files are in omni.ja too.

My main question is if we are going to read more from the omni.ja before first paint of the browser UI. Currently the search service starts asynchronously after first paint, and we have a test ensuring that. Most of the add-on manager initialization happens before first paint.

> Any pointers to tests I could use to verify / measure these issues would be
> very useful, thanks

The impact of I/O is hard to measure unfortunately, as it's not deterministic. Typically this would be more visible on cold startup on machines with slow rotating hard drives.

Comment 4

[Dale Harvey (:daleharvey)]

Hey Andrew whats your thoughts on this, especially with the plan of installing 'all' engines?

Comment 5

[Dale Harvey (:daleharvey)]

Ping Andrew

Hey so we are going to be determining the status of this in terms of 66 vs 67 today and it looks like we would need to be landed by 10th. Is this something you can pick up shortly or is it something I can take with some guidance? 

Cheers
Dale

Comment 6

[Andrew Swan [:aswan]]

I started on this before the holidays and ran into a small snag.  Its at the top of my list though, I expect to finish it in the next day or two.  I'll have an update for you either way at the end of today (US west coast time)

Comment 7

[Andrew Swan [:aswan]]

Attachment: Bug 1512436 Add new built-in addon location r=kmag

Comment 8

[Andrew Swan [:aswan]]

I've attached my work-in-progress.  The new test and existing xpcshell tests are passing but it needs some polish as well as more thorough testing.  I hope to have it ready for review tomorrow.

Comment 9

[Dale Harvey (:daleharvey)]

Attachment: 0001-Testing.patch

Hey Andrew

As per IRC chat, I am testing this with unzipped web extensions in the omni jar and finding that the engines seem to install, but the promise returned never seems to resolve for me

Cheers
Dale

Comment 10

[Dale Harvey (:daleharvey)]

This is possibly my problem, I wanted to test this on a fresh patch but I might need some fixes from my patch to avoid a race here, will report back tonight

Comment 11

[Dale Harvey (:daleharvey)]

Just to confirm, it was a problem on my end, basing my work on top of this patch and it looks to be working well at the moment

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8952c559acfb
Add new built-in addon location r=kmag

Comment 13

[Oana Pop-Rus]

Backout for multiple failures e.g. bc, xpcshell, mochitest chrome

Treeherder link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Csuperseded%2Crunnable&revision=8952c559acfb8e9ef41dd97b8304b15ede5a0c99&selectedJob=222830998

Failure logs:
bc: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222830998&repo=autoland&lineNumber=3326
xpcshell: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222830892&repo=autoland&lineNumber=2261
mochitest-chrome: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222829552&repo=autoland&lineNumber=3477

Backout: https://hg.mozilla.org/integration/autoland/rev/b86c22898543ba5cefed5b453af7ef46dcc1dfb5

Comment 14

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e476cb7c0c0b9baaabfa0b75851ee2a922ec3678
Bug 1512436 Add new built-in addon location r=kmag

Comment 15

[Cristina Coroiu [:ccoroiu]]

Backed out changeset e476cb7c0c0b (bug 1512436) for mochitest failures e.g. at browser/extensions/screenshots/test/browser/browser_screenshots_ui_check.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c486d86fd49fc2079255f8c96d94ec8c1fa2950a

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e476cb7c0c0b9baaabfa0b75851ee2a922ec3678

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222984463&repo=mozilla-inbound&lineNumber=5041
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222988266&repo=mozilla-inbound&lineNumber=2162
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222988690&repo=mozilla-inbound&lineNumber=4317

Comment 16

[Andrew Swan [:aswan]]

I'll fix that test and re-land this after the soft freeze is over.

Comment 17

[Dale Harvey (:daleharvey)]

Andrew asked me to leave a needinfo with some follow ups that would be very useful

1. Add support for hidden: true so we can avoid showing these in about:addons
2. Expose an isSystem or similiar property so we can distinguish built in addons from user installed ones

Many thanks

Comment 18

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e75697e98c28a99c29930012c3c71334b01a82
Bug 1512436 Add new built-in addon location r=kmag

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e3e75697e98c

Comment 20

[Andrew Swan [:aswan]]

The patch above should cover the requests from comment 17, if anything got overlooked, lets open a new bug.

Comment 21

[Madalin Cotetiu]

Can you please add some STRs to this issue or mark it as "qe-verify- " if no manual testing is needed ?

Comment 22

[Andrew Swan [:aswan]]

no manual testing needed, thanks.