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

protocol: provide default response code for custom request jobs #9360

Merged
merged 3 commits into from May 11, 2017

Conversation

Projects
None yet
3 participants
@deepak1556
Member

deepak1556 commented May 3, 2017

Service worker registration requires a successful response code https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_write_to_cache_job.cc?l=285, until https://bugs.chromium.org/p/chromium/issues/detail?id=688481 gets landed we have to provide a default success response code.

Fixes #9354

@concreted

This comment has been minimized.

concreted commented May 11, 2017

@electron/maintainers any chance this can get a review soon? it looks like a small change, and it will unlock using service workers in electron-forge (which relies on this file protocol interception).

@kevinsawicki kevinsawicki self-assigned this May 11, 2017

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 11, 2017

LGTM, just gotta fix up the specs on windows, looks like a path issue, doing that now.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 11, 2017

@deepak1556 saw a weird crash on Windows CI:

ok 466 webContents module webrtc ip policy api can set and get webrtc ip policies
[4804:0511/152835.100:FATAL:wrappable.cc(24)] Check failed: !wrapper_.IsEmpty(). 
Backtrace:
	base::win::GetDataResourceFromModule [0x66A2C1D7+140094]
	base::win::GetDataResourceFromModule [0x66A4FEF7+286814]
	mate::WrappableBase::GetWrapper [0x0108C604+244] (c:\users\electron\workspace\electron-win-ia32\vendor\native_mate\native_mate\wrappable.cc:24)
	mate::EventEmitter<atom::api::WebContents>::GetWrapper [0x010F2E4A+26] (c:\users\electron\workspace\electron-win-ia32\atom\browser\api\event_emitter.h:45)
	mate::EventEmitter<atom::api::WebContents>::EmitWithSender<int> [0x010C5F80+144] (c:\users\electron\workspace\electron-win-ia32\atom\browser\api\event_emitter.h:82)
	mate::EventEmitter<atom::api::WebContents>::Emit<int> [0x010BEFE2+34] (c:\users\electron\workspace\electron-win-ia32\atom\browser\api\event_emitter.h:72)
	atom::api::WebContents::RenderViewDeleted [0x010F84C0+112] (c:\users\electron\workspace\electron-win-ia32\atom\browser\api\atom_api_web_contents.cc:698)
	atom::api::WebContents::~WebContents [0x010DFECB+251] (c:\users\electron\workspace\electron-win-ia32\atom\browser\api\atom_api_web_contents.cc:421)
	atom::api::WebContents::`scalar deleting destructor' [0x010E2B16+22]
	mate::WrappableBase::SecondWeakCallback [0x0108CAF5+69] (c:\users\electron\workspace\electron-win-ia32\vendor\native_mate\native_mate\wrappable.cc:56)
	v8::internal::PerThreadAssertScope<4,1>::PerThreadAssertScope<4,1> [0x6793A4F3+4257698]
	v8::internal::PerThreadAssertScope<4,1>::PerThreadAssertScope<4,1> [0x6793C520+4265935]
	base::win::GetDataResourceFromModule [0x66A2EE6E+151509]
	base::win::GetDataResourceFromModule [0x66A6093F+354982]
	base::win::GetDataResourceFromModule [0x66A5F3DC+349507]
	base::win::GetDataResourceFromModule [0x66A631ED+365396]
	base::win::GetDataResourceFromModule [0x66A63A7A+367585]
	base::win::GetDataResourceFromModule [0x66A6044E+353717]
	base::win::GetDataResourceFromModule [0x66A95C75+572892]
	content::ServiceWorkerHandle::provider_id [0x64A80419+5154104]
	content::ServiceWorkerHandle::provider_id [0x64A8168C+5158827]
	content::ServiceWorkerHandle::provider_id [0x64A7C8C3+5138914]
	content::ServiceWorkerHandle::provider_id [0x65185574+12514963]
	content::ServiceWorkerHandle::provider_id [0x65185493+12514738]
	content::ServiceWorkerHandle::provider_id [0x6518488E+12511661]

Rebased and force pushed, will see if it happens again.

@kevinsawicki kevinsawicki merged commit afe5823 into master May 11, 2017

6 of 9 checks passed

electron-linux-x64 Build #6592375 failed in 155s
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #6592373 succeeded in 130s
Details
electron-linux-ia32 Build #6592374 succeeded in 127s
Details
electron-mas-x64 Build #4163 succeeded in 9 min 58 sec
Details
electron-osx-x64 Build #4167 succeeded in 9 min 24 sec
Details
electron-win-ia32 Build #3148 succeeded in 10 min
Details
electron-win-x64 Build #3122 succeeded in 10 min
Details

@kevinsawicki kevinsawicki deleted the sw_protocol_patch branch May 11, 2017

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 11, 2017

Specs are green, merging this 👍

@deepak1556 let me know if you think that crash was caused by this pull request, it happened before the added specs so I wasn't sure, it was right after ok 466 webContents module webrtc ip policy api can set and get webrtc ip policies finished.

@concreted

This comment has been minimized.

concreted commented May 11, 2017

thanks a ton @kevinsawicki! 💪 will this be included in the next release + when do releases happen?

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented May 11, 2017

will this be included in the next release + when do releases happen?

Yes, usually at least once a week, might be as soon as tomorrow or early next week.

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