665527 - Firefox freezes for multiple seconds when opening about:permissions

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Dão Gottwald [:dao]]

Bug 661662 really only helped marginally, this is still way too slow.

Comment 1

[Javi Rueda :javirid]

Running on an Athlon 64 3200+ (mono-core) runing on a 32 bits Win7, the list at the left populates instantly.

Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110713 Firefox/8.0a1

Comment 2

[:Gavin Sharp [email: [e-mail]]]

This is highly dependent on the number of nsIPermissionManager entries you have stored in your profile, presumably. We might want to implement chunking (similar to what the download manager does for download history).

Comment 3

[:Gavin Sharp [email: [e-mail]]]

relevant download manager code:
http://hg.mozilla.org/mozilla-central/annotate/982a5835fba1/toolkit/mozapps/downloads/content/downloads.js#l1212

Comment 4

[Javi Rueda :javirid]

Modify function loadPermissions()? In file:

http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/permissions.js#326

> relevant download manager code:
> http://hg.mozilla.org/mozilla-central/annotate/982a5835fba1/toolkit/mozapps/
> downloads/content/downloads.js#l1212

And create something similar to the appointed function stepListBuilder?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

The relevant function in this case is the "Services.perms.enumerator" part of enumerateServices in browser/components/preferences/aboutPermissions.js (though I suppose it's also possible for the Services.logins enumerations to be slow).

Comment 6

[Javi Rueda :javirid]

Attachment: Patch for bug 665527

I normally don't do right choosing a reviewer, so sorry if I fail.

Implements chunking of 3 permission items each one with a delay of a maximum of 300.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 551788 [details] [diff] [review]
Patch for bug 665527

I can review a patch for this bug. Unfortunately this patch isn't a patch for this bug (see comment 5) :)

It would be interesting to have people seeing this slowness compare the performance of opening this dialog to the performance of opening about:permissions, though, since they're essentially doing the same thing.

Comment 8

[Dão Gottwald [:dao]]

The exceptions dialog opens reasonably fast for me, doesn't freeze the browser.

By the way, I just killed Firefox after it wouldn't recover after about a minute trying to open about:permissions.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

If you comment out the call to addToSitesList() in addHost(), is it much faster?

Comment 10

[Javi Rueda :javirid]

I have created a new patch but modifying file aboutPermission.js this time. As I have few sites on my configuration, I am not able to test if it is useful to solve the problem.

Is the chunking approach still valid?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

We need to find out where the overhead is before deciding on an approach. Being able to reproduce the problem is key - I suspect you probably could do it by manually creating many nsIPermissionManager entries (or having a third-party program like Spybot S&D do it for you).

Comment 12

[Javi Rueda :javirid]

(In reply to Gavin Sharp from comment #9)
> If you comment out the call to addToSitesList() in addHost(), is it much
> faster?

Commenting out line http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/aboutPermissions.js#565 is faster (not too much, but it doesn't freeze the application in any moment).

Comment 13

[Javi Rueda :javirid]

(In reply to Gavin Sharp from comment #11)
> We need to find out where the overhead is before deciding on an approach.
> Being able to reproduce the problem is key - I suspect you probably could do
> it by manually creating many nsIPermissionManager entries (or having a
> third-party program like Spybot S&D do it for you).

I inserted many permissions in my build by using the inmunizing feature by Spybot. Yeah, now when trying to open the about:permissions it turns the application unresposive and appears the dialog about script being very slow.

About my previous comment for line 565, I tried it with those new permissions already added.

Comment 14

[Felix Fung (:felix)]

Reproduction steps:
Open about:permissions in a new tab
Open Tools > Web Developer > Web Console
Running the following javascript:
// BEGIN

// WARNING MAY NEED TO RESET PERMISSIONS AFTERWARDS
// Generates 10000 random URLS and blocks their popups
for (var a = 0; a < 10000; a++) {
  var s = "";
  for (var i = 0; i < 10; i++) {
    let r = Math.floor(Math.random() * 26);
    s += String.fromCharCode(97 + r);
  }

  var tt = new Site("www." + s + ".com");
  Services.perms.add(tt.httpURI, 'popup', Ci.nsIPermissionManager.DENY_ACTION);
}
// END

Comment 15

[Felix Fung (:felix)]

Attachment: Incrementally Building the about:permissions Sites List

Summary of Changes:
- Using setTimeout, we add delay to incrementally build the siteslist initially
-- We do this for every type of permission
- Removed batching/fragments when adding to siteslist
- When filtering, new items are still being added with only relevant new items being shown

Additional Comments:
- With this proposed change, if there are enough permissions, some users can effectively never reach certain sites since they'll have to wait a rather long time. This isn't a regression per se since they just couldn't access about:permissions now. Something to consider.
- It'll continually build without locking up but eventually just the sheer number of DOM elements will make this rather slow.
- Since people may filter while the list is still building, we should have a throbber or similar to indicate that the list is still building. Otherwise, people may be confused why their searches are missing items.

Comment 16

[Felix Fung (:felix)]

Attachment: Alternative Submit Crash Report Screen With Checkbox

- Made the checkbox show up on the correct side

Comment 17

[Felix Fung (:felix)]

Attachment: Incrementally Building the about:permissions Sites List

Ignore above

Comment 18

[:Margaret Leibovic]

Comment on attachment 559836 [details] [diff] [review]
Incrementally Building the about:permissions Sites List

Review of attachment 559836 [details] [diff] [review]:
-----------------------------------------------------------------

I applied this patch, and it seems to be working as expected. However, I think you should add a test to browser_permissions.js to make sure the sites list is being built correctly. We won't be able to capture the performance impact with a browser chrome test, but you can at least test the functionality with a smaller set of sites.

In comment 15, you mentioned that you think we should have a throbber while the list is building. I don't think you need to do that in this bug, but you should make sure you file a follow-up.

When I first read this patch, I wanted to find a way to combine stepArrayListBuilder and stepEnumListBuilder, since they have some redundant logic. After thinking about it, it seems like they're different enough to justify two different methods, but I would be impressed if you come up with an elegant way to combine them. That's not a requirement for a review+ from me, though.

::: browser/components/preferences/aboutPermissions.js
@@ +528,5 @@
> +          // aLogin.hostname is a string in origin URL format
> +          // (e.g. "http://foo.com")
> +          let uri = NetUtil.newURI(aLogin.hostname);
> +          this.addHost(uri.host);
> +      }, 3);

What is this hard-coded 3 for? Do you mean to be using LIST_BUILD_CHUNK here?

@@ +541,2 @@
>  
> +    permsEnum = Services.perms.enumerator; // doesn't work unless copied

You should put a let in front of this.

@@ +542,5 @@
> +    permsEnum = Services.perms.enumerator; // doesn't work unless copied
> +    setTimeout(this.stepEnumListBuilder.bind(this), 0, permsEnum, 3);
> +  },
> +
> +  stepArrayListBuilder: function(aList, aPos, aFn, aNumItems) {

I think you should write some comments about what these parameters are for and how you're using them, since it isn't immediately obvious.

@@ +556,5 @@
> +      let delay =
> +        Math.min(this.sitesList.children.length * 10, this.LIST_BUILD_DELAY);
> +      setTimeout(this.stepArrayListBuilder.bind(this), delay, aList, aPos + 1,
> +        aFn, this.LIST_BUILD_CHUNK);
> +    }

Some comments about this logic would also be useful.

@@ +568,5 @@
> +        this.addHost(permission.host);
> +      }
> +    } else {
> +      return;
> +    }

I think it's cleaner to check for the return condition, then just continue on with the other code otherwise. This comment also applies to the similar logic at the beginning of stepArrayListBuilder.

Comment 19

[Felix Fung (:felix)]

Attachment: Incrementally Building the about:permissions Sites List

Additional Changes:
- Made all the requested changes above
- All sites objects (via addHost) are added at initialization. Clunking now pertains only to DOM changes. This is better because we can do things to make searching possible without having for the entire list to be built. (Also, we now only need one builder function)!

I was wondering how I would go about testing this? Since the list building relies on setTimeout, the browser_permissions.js tests are going to break and I can't seem to find a way around it. I tried stubbing out setTimeout but I didn't seem to be able to...

Comment 20

[:Margaret Leibovic]

(In reply to Felix Fung from comment #19)
> I was wondering how I would go about testing this? Since the list building
> relies on setTimeout, the browser_permissions.js tests are going to break
> and I can't seem to find a way around it. I tried stubbing out setTimeout
> but I didn't seem to be able to...

You could fire a notification when you're done building the sites list and then just have the test listen for that notification.

Comment 21

[Felix Fung (:felix)]

Attachment: Incr

Comment 22

[Felix Fung (:felix)]

Attachment: Incrementally Building the about:permissions Sites List

- Added/moved observers so that there are two phases to testing about:permissions
-- preInit for when window is loaded but sites-list is not yet completed
-- Initialized now fires after sites-list is fully populated
This also means that the testing code has been refactored a bit.

- Added tests for filtering and removing during the pre-init phase
- Made chunking faster (modified constants)

Comment 23

[Felix Fung (:felix)]

Attachment: Incrementally Building the about:permissions Sites List

Additional Changes:
- The recursion was ugly. Instead we use generators and yield to break execution. The code is MUCH cleaner.
- We also clunk not only the DOM but also the internal service calls to find the modified permissions. There is virtually no more freezing when the tab is first opened. Caveat: For ~3000 items, there is ~1 second where the sites-list only shows All Sites. However, all the UI is functional/not-locking up which in my opinion is most important. This just means we should really get a throbber.

Comment 24

[:Margaret Leibovic]

Comment on attachment 562328 [details] [diff] [review]
Incrementally Building the about:permissions Sites List

Here are some of my initial comments on this patch:

>diff --git a/browser/components/preferences/aboutPermissions.js b/browser/components/preferences/aboutPermissions.js

>+  SERVICE_BUILD_CHUNK: 500, // interval size
>+  SERVICE_BUILD_DELAY: 50, // delay between intervals

>+  DOM_BUILD_CHUNK: 5, // interval size
>+  DOM_BUILD_DELAY: 100, // delay between intervals

Why do you batch building up sitesArray and DOM buildup separately? Seems like it would be simpler to leave them together, as they are now (and like you did in your first patch).

>-   * Creates Site objects for the top-frecency sites in the places database and stores
>+   * Creates Site objects for the top-frequency sites in the places database and stores

frecency is correct! It's a value we compute that represents a combination of frequency/recency.

>   getSitesFromPlaces: function() {
>     gSitesStmt.params.limit = this.PLACES_SITES_LIMIT;
>     gSitesStmt.executeAsync({
>       handleResult: function(aResults) {
>-        AboutPermissions.startSitesListBatch();

The batching still seems useful here (we're adding 50 items at a time).

>       handleCompletion: function(aReason) {
>         // Notify oberservers for testing purposes.
>-        Services.obs.notifyObservers(null, "browser-permissions-initialized", null);
>+        AboutPermissions._initPlacesDone = true;
>+        if (AboutPermissions._initPlacesDone
>+            && AboutPermissions._initServicesDone) {

Checking _initPlacesDone here is unnecessary (same for _initServicesDone in enumerateServicesDriver).

Comment 25

[Felix Fung (:felix)]

> The batching still seems useful here (we're adding 50 items at a time).

I believe I'm adding 5 items at a time?

> Why do you batch building up sitesArray and DOM buildup separately? Seems
> like it would be simpler to leave them together, as they are now (and like
> you did in your first patch).

I forgot to explain earlier (my bad), but I did this with filtering in mind. So we definitely want sitesArray to be built up before and separately from the DOM. The point is that the DOM might take indefinitely (really really) long to complete, the sitesArray, less so. If a user was trying to reach some site that happens at the end of the list, even with filtering, the user may have to wait forever to see it. So I build the sitesArray first, and if the user filters, then we can prioritize creating the dom items that need to be shown. This way we should always be able to 'search' and find in a reasonable amount of time.

> >       handleCompletion: function(aReason) {
> >         // Notify oberservers for testing purposes.
> >-        Services.obs.notifyObservers(null, "browser-permissions-initialized", null);
> >+        AboutPermissions._initPlacesDone = true;
> >+        if (AboutPermissions._initPlacesDone
> >+            && AboutPermissions._initServicesDone) {
> 
> Checking _initPlacesDone here is unnecessary (same for _initServicesDone in
> enumerateServicesDriver).

During testing, when I didn't check for both being done the tests would run on a partially built list and would fail. Since both initPlaces and initServices kind of run asynchronously, I had them check that both are done. Is there an easier way?

Comment 26

[:Margaret Leibovic]

(In reply to Felix Fung (:felix) from comment #25)
> > The batching still seems useful here (we're adding 50 items at a time).
> 
> I believe I'm adding 5 items at a time?

Oh, I guess I mixed up the the different constants. If we decide to only add the delay to the services enumeration like I'm going to suggest below, we can keep the batching for the DOM instead.

> > Why do you batch building up sitesArray and DOM buildup separately? Seems
> > like it would be simpler to leave them together, as they are now (and like
> > you did in your first patch).
> 
> I forgot to explain earlier (my bad), but I did this with filtering in mind.
> So we definitely want sitesArray to be built up before and separately from
> the DOM. The point is that the DOM might take indefinitely (really really)
> long to complete, the sitesArray, less so. If a user was trying to reach
> some site that happens at the end of the list, even with filtering, the user
> may have to wait forever to see it. So I build the sitesArray first, and if
> the user filters, then we can prioritize creating the dom items that need to
> be shown. This way we should always be able to 'search' and find in a
> reasonable amount of time.

I think that this is over-engineering for an edge case. Since we're currently not prioritizing those items during filtering, this code isn't necessary at the moment. If you work on this prioritized filtering in a follow-up bug, you could add complexity there, as it's needed. I think avoiding this would simplify this patch a lot, since you wouldn't need _sitesArray or _buildPos, and you also wouldn't need to modify deleteFromSitesList.

> > >       handleCompletion: function(aReason) {
> > >         // Notify oberservers for testing purposes.
> > >-        Services.obs.notifyObservers(null, "browser-permissions-initialized", null);
> > >+        AboutPermissions._initPlacesDone = true;
> > >+        if (AboutPermissions._initPlacesDone
> > >+            && AboutPermissions._initServicesDone) {
> > 
> > Checking _initPlacesDone here is unnecessary (same for _initServicesDone in
> > enumerateServicesDriver).
> 
> During testing, when I didn't check for both being done the tests would run
> on a partially built list and would fail. Since both initPlaces and
> initServices kind of run asynchronously, I had them check that both are
> done. Is there an easier way?

I just mean in this specific if condition check - you just set AboutPermissions._initPlacesDone to true right before checking it, so it will always be true here :)

Comment 27

[Felix Fung (:felix)]

Attachment: Incrementally Building the about:permissions Sites List

Made the requested changes above.

Comment 28

[:Margaret Leibovic]

Comment on attachment 563863 [details] [diff] [review]
Incrementally Building the about:permissions Sites List

I'm sorry this is taking so long -- I wanted to think about this. It looks like the patch is pretty close to done. However, I think the test requires reworking, which my comments talk about below.

>--- a/browser/components/preferences/aboutPermissions.js
>+++ b/browser/components/preferences/aboutPermissions.js

>+    // We load all the internal representations of site objects first but we
>+    // defer building the DOM sitelist until later and then in parts to prevent
>+    // the UI from locking up.

This comment isn't correct anymore.

>   getSitesFromPlaces: function() {
>     gSitesStmt.params.limit = this.PLACES_SITES_LIMIT;
>     gSitesStmt.executeAsync({
>       handleResult: function(aResults) {
>-        AboutPermissions.startSitesListBatch();
>         let row;
>         while (row = aResults.getNextRow()) {
>           let host = row.getResultByName("host");
>           AboutPermissions.addHost(host);

If we remove the existing batching code, these addHost calls will no longer be batched, so I think we should keep that batching code for the sites from places.

>+  enumerateServicesDriver: function() {
>+    if (this.enumerateServicesGenerator.next()) {
>+      let delay = Math.min(this.sitesList.itemCount * 5, this.LIST_BUILD_DELAY);

Nit: could you throw a comment in here about the reasoning behind this delay math?

>   /**
>    * Finds sites that have non-default permissions and creates Site objects for
>    * them if they are not already stored in _sites.
>    */
>-  enumerateServices: function() {
>-    this.startSitesListBatch();
>+  getEnumerateServicesGenerator: function() {
>+    let itemCnt = 0;

>+      if (((itemCnt + 1) % this.LIST_BUILD_CHUNK) == 0) {

If you initialize itemCnt to 1 instead of 0, you could avoid the + 1 in these if statements, right? Nit: with that change, you could get rid of two sets of parentheses :)

>--- a/browser/components/preferences/tests/browser_permissions.js
>+++ b/browser/components/preferences/tests/browser_permissions.js

> function cleanUp() {

>+  while (gBrowser.tabs.length > 1) {
>+    gBrowser.removeCurrentTab();
>   }
>-
>-  gBrowser.removeTab(gBrowser.selectedTab);
>+  Services.perms.removeAll();

I don't think it's safe to just remove all permissions in your cleanup function -- you should only remove the permissions you add. You could do this with registerCleanUpFunction where you add the permissions. Similarly, you could register cleanup functions to just remove the tabs you add.

> function runNextTest() {

>+  // no setup or pre-init defined, we can keep using the previous window
>+  if (nextTest.setup == null && nextTest.preInit == null) {
>+    nextTest.run();
>+  } else {
>+    while (gBrowser.tabs.length > 1) {
>+      gBrowser.removeCurrentTab();
>+    }

This seems redundant with the cleanUp function above, and I don't see why you would need to do it between test runs.

>+    if (nextTest.setup) {
>+      Services.perms.removeAll();

Like I said above, I don't think this is safe.

>+      waitForClearHistory(nextTest.setup);

Why do you need to clear history between tests? 

I think it's great that you added more test cases, but I think you may have added too much overhead to handle the setup and preInit cases. Since you only need setup to help with the preInit tests, and you only use preInit in two of the test cases, I think this would be a good candidate for a new test file. This way you would avoid trying to shoe-horn all this new logic into a test file that wasn't designed with your test cases in mind. 

Did you add/modify any tests other than test_preinit_filtering and test_preinit_removal? If not, I think the checks you added to aboutPermissions.js before calling browser-permissions-initialized should guarantee this test file would still work as expected, so you could leave it alone.

Comment 29

[Felix Fung (:felix)]

Attachment: Incrementally Building the about:permissions Sites List

Made suggested changes above.

Comment 30

[:Margaret Leibovic]

Comment on attachment 564703 [details] [diff] [review]
Incrementally Building the about:permissions Sites List

The changes to aboutPermissions.js look good! I like that you moved the tests to a new file, but I still think there's room to improve them:

>+++ b/browser/components/preferences/tests/browser_chunk_permissions.js

>+function cleanUp() {
>+  gBrowser.removeTab(gBrowser.selectedTab);

See my comments about this below -- it's confusing that you're only removing one tab in this cleanup function when you're adding more than one tab as the tests are running.

>+  if (nextTest.desc) {
>+    info("[" + nextTest.desc + "] running test");

Nit: You could reformat your info dump to make your descriptions more descriptive. Since they're strings, they can be sentences that say what the test is doing :) Also, I don't think you need that if statement since all your test objects have a desc property.

>+  // no setup or pre-init defined, we can keep using the previous window
>+  if (nextTest.setup == null && nextTest.preInit == null) {
>+    nextTest.run();

It looks like this if statement was just copied over from your last patch, and it's not necessary anymore.

>+  } else {
>+    if (nextTest.setup) {
>+      nextTest.setup();
>+    }

See my comment below -- I think you can get rid of this and just make one setup function for the whole test file.

>+    // open about:permissions
>+    gBrowser.selectedTab = gBrowser.addTab("about:permissions");

I think the best way to ensure you clean up these tabs properly is to just register a cleanup function here to make sure you have one removeTab for each addTab. Something like this should work:

    let tab = gBrowser.selectedTab = gBrowser.addTab("about:permissions");
    registerCleanupFunction(function() {
      gBrowser.removeTab(tab);
    });

If you do this, you should make sure you get rid of the removeTab call in cleanUp.

>+    setup: function() {
>+      // add test history visit
>+      PlacesUtils.history.addVisit(TEST_URI_1, Date.now() * 1000, null,
>+        Ci.nsINavHistoryService.TRANSITION_LINK, false, 0);
>+
>+      // set permissions ourselves to avoid problems with different defaults
>+      // from test harness configuration
>+      for (let type in TEST_PERMS) {
>+        if (type == "password") {
>+          Services.logins.setLoginSavingEnabled(TEST_URI_2.prePath, true);
>+        } else {
>+          // set permissions on a site without history visits to test enumerateServices
>+          Services.perms.add(TEST_URI_2, type, TEST_PERMS[type]);
>+        }
>+      }
>+
>+      Services.perms.add(TEST_URI_3, "popup", TEST_PERMS["popup"]);
>+    },

I don't think this setup function needs to be associated with this test, since the history visit and permissions it sets are also used in the next test. I think that you should just move this out into a setup function that gets called before any of the tests are run, since in actuality this function is just setting up for the all the tests. (As a side note, in general, I think that if you are going to have tests do their own setup, they should also be doing their own cleanup, so that there isn't confusion about what state the browser is in going into the next test.)

>+      // we want to re-open the tab so that chunking occurs again
>+      gBrowser.removeTab(gBrowser.selectedTab);

See my comment above about a better way to clean up tabs.

Comment 31

[Felix Fung (:felix)]

Attachment: Incrementally Building the about:permissions Sites List

Additional Changes:
- Simplified tests/browser_chunk_permissions.js a lot more

I don't know whether just registering all the tab removals at the end is the best cleanup idea. Seems like, if more tests are added, that's a lot of tabs to have open. Punting back.

Comment 32

[:Margaret Leibovic]

(In reply to Felix Fung (:felix) from comment #31)

> I don't know whether just registering all the tab removals at the end is the
> best cleanup idea. Seems like, if more tests are added, that's a lot of tabs
> to have open. Punting back.

Hmm, I'm not sure how many tabs it would take for that to become a problem. No matter what you will have to incur the cost of adding and removing each tab, whether that's at the end of the test or in the middle. I supposed because in this test they're all about:permissions tabs, there could be some cost associated with the observers, but I don't see it being a real problem right now.

Comment 33

[:Margaret Leibovic]

Comment on attachment 565191 [details] [diff] [review]
Incrementally Building the about:permissions Sites List

Looks good!

Comment 34

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1566ade6c03

Comment 35

[Ed Morley [:emorley]]

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