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

Cancel redirects for the same requestIds and urls if originating from the same tabId #1120

Merged
merged 1 commit into from Mar 1, 2018

Conversation

stoically
Copy link
Member

@stoically stoically commented Feb 12, 2018

As mentioned in #1114 there are still cases where two tabs are opened when redirects are involved:

mac22

In this case www.youtube.com is assigned. The initial request goes to http://youtube.com which results in redirects to https://youtube.com and two times https://www.youtube.com - the last request has a different requestId. All have the same tabId.

To fix this all requestIds and urls that reach the point of being "reopened" are marked as canceled in relation to the tabId - or are canceled if already marked as canceled. Instead of cleaning up canceled requests with a timeout, we can use webRequest.onCompleted and webRequest.onErrorOccurred - though, since they are not 100% reliable, a 2seconds backup cleanup timer is included (uncanceling is also covered with tests).

The downside of this fix is that, if the user left clicks http://youtube.com on a website and switches back to the website, while the confirm page or target container is loading and the completed callback, error callback and backup cleanup timer haven't fired, then all clicks in that tab on the urls marked as canceled will be canceled. I'd personally say that is highly unlikely - the requests complete normally really fast, unless the user has a really slow internet-connection, and even if not after 2seconds the requests get uncanceled - and still preferable in comparison to two tabs opening at random.

Fixes #940 #1003 #1080 #728 #912


Some informations when and how the redirects happen:

First time with clean Network cache in Firefox. 3 requests, all have the same requestId

[requestId: 1] http://youtube.com -[network]-> 301 - onBeforeRedirect fires
[requestId: 1] https://youtube.com -[network]-> 301 - onBeforeRedirect fires
[requestId: 1] https://www.youtube.com

image

Second time with filled Network cache in Firefox, 4 requests, last has a different requestId

[requestId: 1] http://youtube.com -[internal]-> 301 - onBeforeRedirect doesn't fire
[requestId: 1] https://youtube.com -[network]-> 301 - onBeforeRedirect fires
[requestId: 1] https://www.youtube.com -[internal]-> 301? - onBeforeRedirect doesn't fire
[requestId: 2] https://www.youtube.com

image

Filed a Firefox bug since I believe this is not intended behavior. If the requestId wouldn't change or we had a way of correlating the two requests, this fix could be made side-effect-free.

@stoically stoically force-pushed the cancel-redirects-tabid-url branch 2 times, most recently from 342217e to 9954e00 Compare February 13, 2018 04:05
stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 18, 2018
stoically added a commit to stoically/multi-account-containers that referenced this pull request Feb 18, 2018
@stoically stoically force-pushed the cancel-redirects-tabid-url branch 5 times, most recently from 6ebc0c9 to 8878f3a Compare February 22, 2018 18:38
@stoically stoically changed the base branch from testing-6.0.0 to master February 23, 2018 13:06
Copy link
Member

@groovecoder groovecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had trouble reproducing the original bug in master, but I know I've seen it before. This code looks good to me, and I tested this branch with no problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants