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

Fix macOS patches #334

Merged
merged 3 commits into from Feb 26, 2018
Merged

Fix macOS patches #334

merged 3 commits into from Feb 26, 2018

Conversation

tectiv3
Copy link
Contributor

@tectiv3 tectiv3 commented Feb 19, 2018

Removed fix-widevine patch
Fixed visibility.cc type error

 no viable conversion from returned value of type 'unique_ptr<base::ListValue, default_delete<base::ListValue>>' to function return type 'unique_ptr<base::Value, default_delete<base::Value>>'

Fixed memory_mapped_file error (probably this error exists only on 10.11)

memory_mapped_file_posix.cc:119:26: error: non-constant-expression cannot be narrowed from type 'size_t' (aka 'unsigned long') to 'off_t' (aka 'long long') in initializer list

@tectiv3 tectiv3 mentioned this pull request Feb 19, 2018
6 tasks
@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 19, 2018

Don't merge it yet please. I think my mapped_file fix isn't really a fix.
Chromium started to hang indefinitely (in uninterruptible state) so only system restart is able to kill it.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 19, 2018

or may be it is a different problem, because according to:
https://chromium.googlesource.com/chromium/src/+/0413fd9f7c660ba7504dade7bb838862f138348e/base/files/memory_mapped_file_posix.cc#104
it is supposed to fallback to manual mode on everything that is not APFS.

So, this version is unstable for some other reason.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 20, 2018

nope. Still crashing. So it's definitely something else.

@Eloston
Copy link
Member

Eloston commented Feb 20, 2018

Alright. Let me know when it's ready to be reviewed.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 20, 2018

It is ready.
The problem is not with my patches. But I need someone else to confirm it.

@kfur
Copy link
Contributor

kfur commented Feb 21, 2018

@tectiv3 I can. Just let me get your build image.

@arutr
Copy link
Contributor

arutr commented Feb 21, 2018

@tectiv3 This PR builds and doesn't crash, but #315 still persists

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 21, 2018

@Artur96 it doesn't crash for me immediately. First time it did after a couple of hours of actively using it.

@Eloston Eloston added this to the 64.x.x.x milestone Feb 22, 2018
@Eloston
Copy link
Member

Eloston commented Feb 22, 2018

The formatting looks fine, but I don't have the time to review the changes themselves to see how it works.

If you know how to reproduce the issue, you could try building with debug symbols and running a debugger. If you aren't able to diagnose the issue or resolve it, I can merge the changes and then create a new issue for this problem.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 23, 2018

@Artur96 it's been two days, how does it work for you so far?

@arutr
Copy link
Contributor

arutr commented Feb 23, 2018

@tectiv3 I've only started using UGC 64 today with a crashpad handler patch I've written yesterday.
EDIT: The browser seems to crash due to freeing an unallocated pointer in TemplateURLRef.

@Eloston
Copy link
Member

Eloston commented Feb 23, 2018

@Artur96 Do you have a method to reproduce the crash or a stack trace? I would like to see if that crash will happen on my build.

The only patches I can tell that deal with TemplateURLRef directly are inox-patchset/0008-restore-classic-ntp.patch and ungoogled-chromium/disable-google-host-detection.patch. Though, nothing particularly stands out that could cause a crash in either of them.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 23, 2018

@Artur96 does it actually crash? Because for me it's just freezing and ps reporting main chromium process is in uninterruptible wait (UE status).

@arutr
Copy link
Contributor

arutr commented Feb 23, 2018

@tectiv3 It did actually crash, I have a crash report.
@Eloston Unfortunately, not really. I was using the omnibox to perform a search.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 23, 2018

for me it also freezing on search

@Eloston
Copy link
Member

Eloston commented Feb 23, 2018

Could you guys try removing ungoogled-chromium/add-flag-for-search-engine-collection.patch and see if the crash still happens? If it does, try different combinations that include ungoogled-chromium/disable-google-host-detection.patch and patches/inox-patchset/0008-restore-classic-ntp.patch (these last two patches involve Template URLs in some form, but they don't modify them to the same level as the search collection patch).

Also, does the freezing happen consistently on every search, are there some fixed conditions that make it freeze/crash, or is it seemingly random?

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 23, 2018

I am not sure, cause first time I built and run it - it was couple of hours before it froze, but now I do remember it was on search.

I'll try it now.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 24, 2018

Disabled ungoogled-chromium/add-flag-for-search-engine-collection.patch.
So far so good...

@Eloston
Copy link
Member

Eloston commented Feb 24, 2018

Hmm, If @Artur96 can confirm, then I will investigate the patch.

@arutr
Copy link
Contributor

arutr commented Feb 24, 2018

@Eloston So far the most probable way of inducing the crash is by typing webrtc test into the omnibar and clicking on the first result in DuckDuckGo. I'll try building without the mentioned patch and see if the crash still occurs.

@Eloston
Copy link
Member

Eloston commented Feb 24, 2018

@Artur96 What is that first result that you find on DuckDuckGo? Perhaps that page is causing the issue.

@arutr
Copy link
Contributor

arutr commented Feb 24, 2018

@Eloston https://test.webrtc.org/, it doesn't always cause a crash.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 25, 2018

Still good on my side, doesn't crash on any DuckDuckGo query.
@Artur96 can you please upload your crashpad patch? I'm very eager to try it.

@proteo
Copy link

proteo commented Feb 25, 2018

Sorry to interrupt you guys, just wanted to let you know how much we, Mac users, appreciate your efforts. Ungoogled Chromium has been my main browser for a while now.
Thanks a a lot for your hard work!

@arutr
Copy link
Contributor

arutr commented Feb 25, 2018

@tectiv3 I've submitted a PR to your fork

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 26, 2018

@Artur96 running it now, all good!

@tectiv3
Copy link
Contributor Author

tectiv3 commented Feb 26, 2018

I've uploaded my build here: https://github.com/tectiv3/ungoogled-chromium-binaries/releases/tag/64.0.3282.186-1

@Eloston
Copy link
Member

Eloston commented Feb 26, 2018

Alright, so here's what I'm going to do: I'll merge this PR first, and then @Artur96's fix for crashpad can be submitted as another PR. Then, I will create another issue to deal with the search collection bug.

@Eloston Eloston merged commit 3523360 into ungoogled-software:develop Feb 26, 2018
@arutr
Copy link
Contributor

arutr commented Feb 26, 2018

@tectiv3 removing ungoogled-chromium/add-flag-for-search-engine-collection.patch does fix the crash.

@Eloston
Copy link
Member

Eloston commented Mar 1, 2018

@tectiv3 @Artur96 If you guys could check out #338, that would be great. I'm close to finishing the other changes for 64.0.3282.186-1, and would like to ensure that macOS support works.

@tectiv3
Copy link
Contributor Author

tectiv3 commented Mar 1, 2018

okay, building it now

@tectiv3
Copy link
Contributor Author

tectiv3 commented Mar 1, 2018

So far so good.

Wyse- pushed a commit to Wyse-/ungoogled-chromium that referenced this pull request Jul 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants