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

Use landing page even if breakonload is disabled #613

Merged
merged 1 commit into from Mar 10, 2018

Conversation

Projects
None yet
4 participants
@rakatyal
Copy link
Member

rakatyal commented Mar 9, 2018

As Diego mentioned in the comment, this is required to avoid the race condition during redirections inside VS:
Some web-pages redirect very fast to other web-pages etc... You go to localhost:1234/index.htm and you immediately get redirected to localhost:1234/login.htm
When that happens, chrome-core won't be able to find the url in the list of targets supplied by chrome, and it will fail... If we use the landing page we avoid this race condition

@rakatyal rakatyal requested a review from digeff Mar 9, 2018

@auchenberg

This comment has been minimized.

Copy link
Contributor

auchenberg commented Mar 9, 2018

Why? What's the scenario that requires this?

@digeff

This comment has been minimized.

Copy link
Contributor

digeff commented Mar 9, 2018

@auchenberg Some web-pages redirect very fast to other web-pages etc... You go to localhost:1234/index.htm and you immediately get redirected to localhost:1234/login.htm
When that happens, chrome-core won't be able to find the url in the list of targets supplied by chrome, and it will fail... If we use the landing page we avoid this race condition

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 9, 2018

Even though we just talked about this offline, please include some description and context when filing PRs here, this is an open source project and I want to document things publicly. Also I trust github more than I trust SfB to not lose old conversations :)

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 9, 2018

When breakOnLoad is enabled, the target page blinks and shows the "Paused in..." message briefly when starting up. I thought we had something for it to not show that overlay initially?

That makes me not want to merge this yet, users would see that blink in every scenario.

@rakatyal

This comment has been minimized.

Copy link
Member

rakatyal commented Mar 9, 2018

@roblourens: You are right but how will this PR impact that scenario? We won't blink if user hasn't set any breakOnLoadStrategy with this change. We will add a landing page but that itself won't cause any breakOnLoad breakpoints correct?
We did make a change which would solve that issue for some of the cases when using "regex" strategy but we didn't make any change for 'instrument' strategy.

@rakatyal rakatyal force-pushed the rakatyal_landing branch from ac482ec to ad3f470 Mar 9, 2018

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 10, 2018

Oh right, it won't set up the breakOnLoad BP, just do a very quick navigate.

@roblourens roblourens merged commit f356571 into master Mar 10, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@roblourens roblourens added this to the March 2018 milestone Apr 3, 2018

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