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 crash in custom protocols caused by bad callback exec #10918

Merged
merged 1 commit into from Oct 26, 2017

Conversation

Projects
None yet
4 participants
@pfrazee
Contributor

pfrazee commented Oct 26, 2017

I'm proposing this solution to the crash bug that we've been experiencing in Beaker when using custom protocols.

History:

  • There was a PR by @deepak1556 to solve this: #9358. That got reverted due to a regression (#9587).
  • Robo's suggested cause was "Race between destruction of source and sink buffers caused UAF crash." I wasn't able to confirm or disprove that (I'm still pretty new at this codebase).
  • This is the function in the chromium source that crashes:
// url_request_job.cc
void URLRequestJob::ReadRawDataComplete(int result) {
  DCHECK(request_->status().is_io_pending());
  DCHECK_NE(ERR_IO_PENDING, result);

  // The headers should be complete before reads complete
  DCHECK(has_handled_response_);

  GatherRawReadStats(result);

  // Notify SourceStream.
  DCHECK(!read_raw_callback_.is_null());

  base::ResetAndReturn(&read_raw_callback_).Run(result);
  // |this| may be destroyed at this point.
}
  • It's being called by URLRequestFetchJob::OnURLFetchComplete() in electron. Through logging, I found that the crashes only occurred when ReadRawDataComplete() was called when request_->status().is_io_pending() == false.
  • Based on my read of URLRequestJob, I believe ReadRawDataComplete() only needs to be called if an async ReadRawData() is still in progress. I think the crash is occurring because there's no async read in progress, and ReadRawDataComplete() is blindly calling read_raw_callback_ when it's a null reference. (However, because read_raw_callback_ is private and I dont know how to do custom libchromiumcontent builds yet, I couldn't check its value to confirm that.)
  • The DCHECK against is_io_pending() at the top of ReadRawDataComplete() seems to confirm the check I'm doing.

Test case: https://github.com/pfrazee/electron-bug-video-stream-crash

git clone https://github.com/pfrazee/electron-bug-video-stream-crash
npm install
npm start

Closes #9342. (Related: #8871 #9635)

@pfrazee pfrazee requested a review from electron/reviewers as a code owner Oct 26, 2017

@deepak1556

👍

@deepak1556 deepak1556 requested a review from zcbenz Oct 26, 2017

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Oct 26, 2017

Although this fixes the crash, there will still be problem with playing video through URLFetcher since range headers aren't respected. Should be fixed separately.

@zcbenz

zcbenz approved these changes Oct 26, 2017

@zcbenz zcbenz merged commit 3230048 into electron:master Oct 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@creationix

This comment has been minimized.

creationix commented Oct 26, 2017

Yeah, range headers are kinda a big deal with streaming videos.

@pfrazee

This comment has been minimized.

Contributor

pfrazee commented Oct 26, 2017

FWIW I didn't observe any issues with the builtin HTML5 video player, with the custom server correctly handling ranges. It was able to skip around the timeline and didn't appear to download more than needed to do so.

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Oct 26, 2017

@pfrazee I tried the test case in the PR, it freezes for me after a few seconds and skipping the timeline is not possible. If you check the net logs for loading video over custom and http, you can see that the ranges are messed up.

@pfrazee

This comment has been minimized.

Contributor

pfrazee commented Oct 26, 2017

@deepak1556 Oh you know what, I had a bug in my test demo's range handling that was fixed locally but not pushed. Just pushed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment