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

Webview crashes on Slack calls #481

Closed
srirambv opened this issue Jul 4, 2018 · 12 comments
Closed

Webview crashes on Slack calls #481

srirambv opened this issue Jul 4, 2018 · 12 comments
Assignees
Labels
browser-laptop-parity crash/webview Only tab webview crash. Browser doesn't crash QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes webcompat/not-shields-related Sites are breaking because of something other than Shields.

Comments

@srirambv
Copy link
Contributor

srirambv commented Jul 4, 2018

Description

Webview crashes on Slack calls

Steps to Reproduce

  1. Login to Slack
  2. Ask some one in team to call you
  3. Answer the call and be angry as web view crashes

Actual result:

Webview crashes on Slack calls

Expected result:

Should not cause webview crash

Reproduces how often:

Easily reproduces

Brave version (about:brave info)

Release build and possibly packaged build as well

Reproducible on current release:

Website problems only:

  • Does the issue resolve itself when disabling Brave Shields?
  • Is the issue reproducible on the latest version of Chrome?

Additional Information

cc: @bbondy

@srirambv srirambv added crash/webview Only tab webview crash. Browser doesn't crash browser-laptop-parity webcompat/not-shields-related Sites are breaking because of something other than Shields. labels Jul 4, 2018
@srirambv srirambv added this to the Backlog milestone Jul 4, 2018
@bbondy bbondy modified the milestones: Backlog, Releasable builds Jul 5, 2018
@srirambv
Copy link
Contributor Author

Update: Slack tab keeps crashing at random times. Usually the page becomes unresponsive and then goes into an infinite reload. I keep getting the notification sounds of new messages but doesn't allow to load. Have to open slack in another tab to start using it.

Also experienced this on release build
https://bravesoftware.slack.com/archives/C7VLGSR55/p1530881407000151

@emerick
Copy link
Contributor

emerick commented Jul 20, 2018

Repro'ed on my Windows machine. Looking at the crash dump, it's dying when trying to initialize the WebRTC voice engine, specifically when cricket::WebRTCVoiceEngine::Init calls webrtc::adm_helpers::Init.

@emerick
Copy link
Contributor

emerick commented Jul 20, 2018

It's trying to log an error message when it fails: "[3880:12236:0720/091113.186:ERROR:adm_helpers.cc(73)] Failed to query stereo recording." The crash seems to happen in the call to fwrite, due to an invalid parameter. Hard to see exactly what went haywire, as I'm lacking source code at that point. Almost seems like the WebRTC logging isn't quite configured right and is dying when trying to access the log file.

@garrettr
Copy link
Contributor

I tried to reproduce this on macOS with a debug build but Slack kept crashing before I could even message someone to initiate a test call. I will file issues for the crashes I observed in the debug build, and attempt to repro again with a release build.

@emerick
Copy link
Contributor

emerick commented Jul 27, 2018

@bbondy mentioned that this could be due to fingerprinting-related patches to RTC.

Ran two tests today against the latest master. With "Fingerprinting Protection" set to "Block All Fingerprinting", Brave hung (but did not crash) and then Slack showed a "trouble connecting to the server" page. With the setting switched to "Allow All Fingerprinting", the call went through perfectly fine.

Similar results when visiting https://bravesoftware.slack.com/help/test/calls. "Block All Fingerprinting" led to test failures for microphone and camera. "Allow All Fingerprinting" led to all tests passing.

@emerick
Copy link
Contributor

emerick commented Jul 27, 2018

@garrettr You were possibly crashing in debug mode due to this problematic DCHECK that was recently commented out in Chromium's trunk (but is still enabled in our version of the code):

https://chromium-review.googlesource.com/c/chromium/src/+/1083675

Just an FYI, as I recently ran into it too...

@emerick
Copy link
Contributor

emerick commented Jul 31, 2018

@srirambv I ran a few more tests this morning and I think this is working as expected now, but let me know what you think. There is no longer any crash (possibly due to the move to C68). Everything works as expected when shields are down. When shields are up, the call isn't able to proceed and Slack issues an error message about network connectivity. Not sure there is a much better response we could have in that situation yet still prevent browser fingerprinting.

@srirambv
Copy link
Contributor Author

srirambv commented Aug 2, 2018

Still happens on 1532e51 when running using npm run start Release. Both Slack tab and the call tab crashes once the call is received. But running src/out/Release/brave.exe doesn't crash and allows to answer the call with shields up (default settings).

@emerick
Copy link
Contributor

emerick commented Aug 2, 2018

Seems related to the logging crash, which I'm investigating further now.

@emerick
Copy link
Contributor

emerick commented Aug 2, 2018

So here's my take: This is crashing because we're launching from a non-standard shell (i.e., not cmd.exe). WebRTC tries to log to stderr when it encounters any warnings/errors and non-standard shells are most likely not equipped to properly deal with stderr in this way. This would explain why we see the problem when running with npm run start Release yet not when running brave.exe directly with no additional parameters.

Also, I was able to repro the problem in the latest Chrome by launching Chrome via chrome.exe --enable-logging --v=2 from a non-standard shell (a Git for Windows prompt, in my case). On connecting to a call in Slack, Chrome promptly crashed.

I think the solution in this particular case is to only run this type of test from cmd.exe or by double-clicking on the executable in Windows Explorer. Any other methods are probably unsafe/unreliable.

@emerick
Copy link
Contributor

emerick commented Aug 3, 2018

Spoke with @srirambv and we think the non-native shell is the root cause as discussed above, so closing this issue. Will reopen if we ever encounter this while using cmd.exe.

@emerick emerick closed this as completed Aug 3, 2018
@bbondy bbondy added the QA/Yes label Aug 18, 2018
@srirambv
Copy link
Contributor Author

srirambv commented Sep 24, 2018

Verification Passed on

Brave 0.55.6 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision 16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}
OS Linux
  • Verified calling another user and disconnecting the call, no crash
  • Verified calling another user and user disconnect the call , no crash
  • Verified receiving call from other user and disconnecting the call, no crash
  • Verified receiving call from other user and user disconnect the call , no crash

Went through verification using the following build under macOS 10.13.6 x64 - PASSED

Brave 0.55.6 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision 16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}

Verification PASSED on

Brave 0.55.6 Chromium: 70.0.3538.16 (Official Build) dev (64-bit)
Revision 16ed95b41bb05e565b11fb66ac33c660b721f778-refs/branch-heads/3538@{#306}
OS Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser-laptop-parity crash/webview Only tab webview crash. Browser doesn't crash QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes webcompat/not-shields-related Sites are breaking because of something other than Shields.
Projects
None yet
Development

No branches or pull requests

7 participants