Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v.8.8.1 significantly impacts Safari performance #492

Closed
Bustycat opened this issue Jun 18, 2024 · 20 comments · Fixed by #505
Closed

v.8.8.1 significantly impacts Safari performance #492

Bustycat opened this issue Jun 18, 2024 · 20 comments · Fixed by #505
Assignees
Labels
bug Something isn't working

Comments

@Bustycat
Copy link

Bustycat commented Jun 18, 2024

Expected Behavior

  • Smooth performance

Actual Behavior

  • Safari becomes very stuttering and could crash on iOS and iPadOS sometimes.
  • My iPhone and iPad are both heated.

Steps to Reproduce the Problem

  1. Update uBlacklist to v.8.8.1 on iOS, iPadOS or macOS
  2. Make sure uBlacklist enabled for Safari

Specifications

  • Browser: Safari v17.5
  • Other extensions/add-ons: (unnecessary)
@HoneyLuka
Copy link
Contributor

I didn't test it in detail, but there are some problems with the performance. I tested it on mac, and when I opened the new tab (command + T), I would feel an obvious delay, and the problem disappeared after disabling uBlacklist.

@frank-f
Copy link

frank-f commented Jun 18, 2024

For me on top of the stuttering, Safari crashes every time I close the last Tab or try to open a new one.

@HoneyLuka
Copy link
Contributor

I installed the previous version (8.7.1) and there is no performance problem

@Bustycat
Copy link
Author

Bustycat commented Jun 18, 2024

I didn't test it in detail, but there are some problems with the performance. I tested it on mac, and when I opened the new tab (command + T), I would feel an obvious delay, and the problem disappeared after disabling uBlacklist.

The Settings app on iOS/iPadOS can be also stuttering when uBlacklist is enabled right on the Safari section.

@r-thomson
Copy link

I have an older iPhone and with this latest update it's enough to crash Safari consistently.

@Bustycat
Copy link
Author

I have an older iPhone and with this latest update it's enough to crash Safari consistently.

I tested with iPhone 15 Plus and iPad mini 6. They are a bit outdated actually.

@HoneyLuka
Copy link
Contributor

I have already submit a new version to rollback the code to 8.7.1

@tsm55555
Copy link

For me on top of the stuttering, Safari crashes every time I close the last Tab or try to open a new one.

Same here. Safari crashes every time I try to switch profiles or groups. Also, after I search for something on Google, I cannot click the search results, and my phone starts getting really hot. I'm using iPhone 15 Pro on iOS 17.5.1.

@iorate iorate self-assigned this Jun 19, 2024
@iorate iorate added the bug Something isn't working label Jun 19, 2024
@iorate
Copy link
Owner

iorate commented Jun 19, 2024

@HoneyLuka I appreciate your prompt action.

The problem occurs with my own build too. When Safari crashes, Xcode reports that "uBlacklist" was killed by OS because of too much memory, but not always. I guess some code introduced by #490 (my own code or dependencies) has a problem with Safari, but I have no idea what it is. Interestingly, the problem cannot be reproduced with iOS simulator (17.5).
@HoneyLuka Do you have any thoughts?

If the problem cannot be solved immediately, we may need to create a branch for Safari based on v8.7.1 and cherry-pick bug fixes of the main branch.

@HoneyLuka
Copy link
Contributor

@HoneyLuka I appreciate your prompt action.

The problem occurs with my own build too. When Safari crashes, Xcode reports that "uBlacklist" was killed by OS because of too much memory, but not always. I guess some code introduced by #490 (my own code or dependencies) has a problem with Safari, but I have no idea what it is. Interestingly, the problem cannot be reproduced with iOS simulator (17.5). @HoneyLuka Do you have any thoughts?

If the problem cannot be solved immediately, we may need to create a branch for Safari based on v8.7.1 and cherry-pick bug fixes of the main branch.

After two hours of debugging, I still couldn't find the root cause of the problem. As you mentioned, the problematic version causes Safari's memory usage to be significantly high. I tried upgrading or downgrading the dependencies in package.json, but the issue wasn't resolved.

This memory issue occurs even when Safari is left with just a blank page. Therefore, I think we can narrow down the problem by gradually disabling the program's functional modules. If disabling a particular module causes Safari's memory usage to return to normal, then we have identified that the issue lies within that module. After that, it will be much easier to continue to go deep into the module to locate specific problems.

For example, we could directly disable the cloud sync module. If we find that Safari's memory usage returns to normal once this module is disabled, then we have pinpointed the problem.

Because I'm not very familiar with the project, I don't have a clear idea of where to quickly disable the entire module. So, I think it would be faster if you do this.

I apologize for being occupied with work-related matters recently, which has limited my availability to focus on this issue.

@iorate iorate pinned this issue Jun 23, 2024
@iorate
Copy link
Owner

iorate commented Jun 23, 2024

I’ve observed that even after completely disabling the background page, the problem persists. On the other hand, it doesn’t occur when the content script is empty. However, identifying the specific code in the content script causing the problem remains unclear. It seems to me that gradually applying changes in #490 leads to a gradual increase in memory usage. Additionally, reducing the number of entries in the content_scripts field in manifest.json mitigates the problem.

I’ve also noticed that enabling the extension via ‘Always Allow on Every Website’ causes the settings page in Safari settings (not the extension’s options page) to become very slow and display incorrect permission information. To be specific, the dropdowns show ‘Ask’ even though they actually behave as ‘Allow’. After encountering this issue, every page—regardless of its relation to the extension—consumes a significant amount of memory. Escaping this state is challenging because Safari remembers the ‘Always Allow on Every Website’ setting, which cannot be reset by reinstalling the extension. I haven’t encountered this problem on my iPad, where the extension is enabled via ‘Always Allow on This Website’.
Interestingly, this phenomenon persists even with v8.8.2 (the same as v8.7.1) to some extent. With my iPhone, enabling ‘Always Allow on Every Website’ made the settings page slow, and every page exhibits high memory usage (hundreds of megabytes).

My hypothesis is that this issue is related to a memory leak bug of Safari concerning content script permissions (possibly related to issue #398 ?) and the memory usage increase in v8.8.0 has brought this bug to light.

@HoneyLuka, I apologize for interrupting your busy schedule, but could you please investigate whether 'Always Allow on This Website' does not trigger the bug and ‘Always Allow on Every Website’ does? Perhaps you need to build v8.8.1 with a different bundle ID to reset the permission settings.

@HoneyLuka
Copy link
Contributor

According to your report, I had do some tests. Like you said, if I select 'Always Allow on This Website', there won't be any memory issues, but if I choose 'Always Allow on Every Website', it does.

After trying a few times, I found the issue lies not in 'Always Allow on This Website' or 'Always Allow on Every Website', but in the length of the matches list in the manifest file.

google: {
    contentScripts: [
      {
        matches: [
          "https://www.google.com/search?*",
          "https://www.google.ad/search?*",
        ]
      }
    ]
}

bing: {
    contentScripts: [
      {
        matches: [
          "https://www.bing.com/search?*",
          "https://www.bing.com/images/search?*",
        ]
      }
    ]
}

etc...

If I keep only a few matches rules in the manifest file as shown above, then select 'Always Allow on Every Website' is also fine.

So the key point is that each enabled matches rule contributes to the increasing severity of this memory issue.

The following is my speculation (not verified in practice), provided for reference only.

  1. Maybe every enabled matches will trigger the content-script, even this page was not loaded at all? (can we print some log in content-script file to verify this?)
  2. If above is true, maybe some memory leak in content-script. We can find the issue by debugging the code in the content-script in depth.(disable functions one by one)
  3. If a blank content-script file still can't solve the problem. This means that 1 and 2 are wrong
  4. Is it possible to avoid this issue by reducing matches list in manifest?

Thank you very much for your continued attention to this difficult issue.
My help on this issue is very limited. If you need me do some check work, feel free to leave a message.

@iorate
Copy link
Owner

iorate commented Jun 24, 2024

@HoneyLuka Thank you for your rapid response!

  1. Maybe every enabled matches will trigger the content-script, even this page was not loaded at all? (can we print some log in content-script file to verify this?)

As far as I tested, the content script is not loaded when search engine result pages (SERPs) are not loaded. I added code that modifies DOM, calls console.log(), and sends blocklists to the background page to the content script, but it had no effect unless SERPs are loaded.
Nonetheless, memory usage on non-SERP pages increases significantly.

  1. If above is true, maybe some memory leak in content-script. We can find the issue by debugging the code in the content-script in depth.(disable functions one by one)
  2. If a blank content-script file still can't solve the problem. This means that 1 and 2 are wrong

I'm not sure if 1. is true, but since the problem does not occur with an empty content script, I believe that the content of the content script is somehow affecting the bug. I will continue to look for code that increases memory usage.

  1. Is it possible to avoid this issue by reducing matches list in manifest?

I think so, but how do we select which matches to remove...?

@iorate
Copy link
Owner

iorate commented Jun 29, 2024

Perhaps simply replacing the content script with a short script that imports it might fix the problem...?

import-content-script.js

import("./content-script.js");

@HoneyLuka Would you please try #505 , which is based on v8.8.1 and introduces import-content-script.js?

@HoneyLuka
Copy link
Contributor

Perhaps simply replacing the content script with a short script that imports it might fix the problem...?

import-content-script.js

import("./content-script.js");

@HoneyLuka Would you please try #505 , which is based on v8.8.1 and introduces import-content-script.js?

After my test, I think this issue has been completely resolved, and the memory usage is even more efficient than in the previous version.

It just ... magic.

After this, I think safari version can be updated normally
Thank you for your hard work :)

@iorate
Copy link
Owner

iorate commented Jun 30, 2024

@HoneyLuka Thank you for testing the solution so quickly! I'll merge it soon.

@aug-dev
Copy link
Contributor

aug-dev commented Jun 30, 2024

Perhaps simply replacing the content script with a short script that imports it might fix the problem...?

No way! 😅

I was regularly checking this issue, and to think that this would be the solution haha.

Software development can be so weird sometimes.

@iorate iorate unpinned this issue Jul 7, 2024
@Bustycat
Copy link
Author

Bustycat commented Nov 7, 2024

Is it just my Safari 18 that the performance becomes worse again?

@HoneyLuka
Copy link
Contributor

Is it just my Safari 18 that the performance becomes worse again?

I checked it and it seems normal
macOS 14.7.1
Safari 18.1

@Bustycat
Copy link
Author

Is it just my Safari 18 that the performance becomes worse again?

I checked it and it seems normal macOS 14.7.1 Safari 18.1

Seems that was simply caused by a not-found subscription.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants