grenzor commented 6 years ago

Actual behavior

Middle mouse click doesn't always open the middle clicked link in a new temporary container rather it just opens the link in the same container it was middle clicked from.

Expected behavior

If global setting to always open temporary container for every middle mouse click is set it should open the middle clicked link in a new temporary container every time.

Steps to reproduce

Set automatic mode, global isolation setting to always open temporary container for every middle mouse click.
Middle click tab on a link on any website such as the 'Issues' tab or project name link on Github 10 times (more if you don't come across bug yet). You should see that some containers may open in new containers but some (or a lot..there doesn't seem to be a pattern to how many will open in new container) will just open in the same container it was middle clicked from.

stoically commented 5 years ago

As mentioned in the wiki:

Not all Mouse Clicks can get catched since on some websites you don't actually click links, but instead other HTML elements and the website then executes arbitrary JavaScript which might open another website or a new tab without opening it in a new Temporary Container.

This is true for navigation on GitHub as well. Nothing I can do about it.

grenzor commented 5 years ago

@stoically

I should have mentioned that I have JS disabled on Github when I tested this (so not fetch/xhr related). I can still reproduce this in the latest FF.

stoically commented 5 years ago

With javascript disabled it should work. Unfortunately I wasn't able to reproduce this. Could you provide a debug log for the case in which it happens?

grenzor commented 5 years ago

log: https://send.firefox.com/download/9a15f3787e/#3VztKTlyvtVGCc0K9TNkOQ

Opened 3 link from this issue's tab (id 6964), clicked Github 'Issues' tab with middle click 3 times to open 3 new tabs, two tabs opened as id 6964 and one tab opened as id 6970.

stoically commented 5 years ago

Thanks for the log. I can see now what's happening. TC cleans clicked links up after 1second and since the key is the URL the following clicks are not handled correctly since they don't take the handleClickedLink path. Can you confirm that this only happens when clicking the same link multiple times in a short period of time?

grenzor commented 5 years ago

It can also happen when you middle-click a link just once. Just tested by middle-clicking Github Issues tab, checking the new tab's container ID name, if it wasn't the same ID then close tab and repeat until ID was the same as tab in which it was middle-clicked from.

Maybe it does have to do with opening and closing a link in short period of time but from memory I remember just middle-clicking a link once and noticing the container ID still was the same and didn't change

stoically commented 5 years ago

So far the log only revealed the case where the same URL is clicked multiple times in rapid succession (<1s). Since I'm unable to reproduce this with just one click I would need another debug log for the cases where this happens.

However, it could be that the reason is/was a race-condition - TC handles clicks with the native onClick handler in a content script and sends the click using Firefox's sendMessage to the background process where it's stored in linksClicked. Now if the tab actually makes the request TC compares the requested URL in onBeforeRequest against linksClicked and if they match handles it further. If for some reason Firefox now calls the request handler before handling the clicked link message it will not work as expected. That's why "Navigating in Tabs"-Isolation is the reliable choice because it doesn't rely on sending a message from the content script.

grenzor commented 5 years ago

log 2: https://send.firefox.com/download/75e0cb14e8/#FTGzKwYN4miIWS4SiMta8A

Process: Middle clicked a new tab by clicking Github 'Issues' from container id 7936, then check container id of new tab and close tab if it isn't id 7936 (should say I checked container id of tab then closed it when id didn't match before the tab fully loaded for most of the tabs if that matters), repeated ~30 times and the last middle-clicked tab showed as container id 7936 even after it was fully loaded.

It may be some kind of race condition. I also noticed when doing the above process that some of the instances where I middle clicked to create a new tab the new tab loaded as id 7936 (id of tab from which it was clicked) then changed to a newer container id in a split second. Not sure if there is some kind of security concern with this since even though the tab changes to a newer id it still showed it changing from the old container id first which makes me wonder if there's a possibility some data could leak between the containers.

grenzor commented 5 years ago

above link reupload: https://send.firefox.com/download/f029d090a6/#NGiHiBC5hASfWrNyteFv6w

stoically commented 5 years ago

then changed to a newer container id in a split second

That's how TC works. What happens if you middle-click the link, is that Firefox opens a new tab in the same container as the currently open one - only if the new tab starts the actual network request TC hops in, potentially cancels and reopens the tab in a new container. So from a/security privacy perspective that's fine, since the request is canceled.

repeated ~30 times

From how you describe it, it sounds like the same <1s race-condition scenario. To be sure I'd need a debug log, unfortunately the send.firefox link is expired. Could you send it again (or maybe attach to the comment)? Thanks!

grenzor commented 5 years ago

Log: https://send.firefox.com/download/d69e70ea37d111ac/#9KZk0OA8G1-PQZiltBbedQ

stoically commented 5 years ago

Thanks. Unfortunately I can't find anything related to the issue in that log. It contains 7x "message received" from the clicks, 7x "created new tab" and 14x "incoming request" (7x for the request that gets canceled/reopened because of the click and 7x for the reopened tab itself which doesn't get canceled/reopened).

grenzor commented 5 years ago

Hmm, not sure what to say. Was the log incomplete or was it not relevant at all?

That log was the whole log and should correspond to the process I mentioned in https://github.com/stoically/temporary-containers/issues/184#issuecomment-462204099 (unless I somehow debugged wrongly or copied an incomplete log when I did the debug). I could try debug it again if you want but since you think it might be the same race condition problem from the first log maybe I should wait for the fix for that and try to reproduce/debug this after that?

stoically commented 5 years ago

Seems incomplete. You said ~30 clicks, but the log contains ~7 clicks, and also the mentioned container number 7936 isn't there. I have an idea to decrease the chance for the race-condition a bit with a workaround - though, it can't get fixed completely. However, you said that it also happens sometimes with only one click, for that scenario a debug log would be really interesting, but I guess that's not that easy to obtain.

stoically commented 5 years ago

v1.0beta4 includes a refactoring that hopefully makes the middle click bug an exception. Let me know if it still doesn't work as expected.

grenzor commented 5 years ago

Thanks @stoically. From testing the beta a little I can't reproduce the issue when multiple links are clicked in succession. I'll keep testing to see if I come across the issue when doing only one click.

grenzor commented 5 years ago

@stoically I was able to reproduce the same multiple links clicked issue using 1.0beta6 (to me it seems more frequent with middle click now)

stoically commented 5 years ago

Well, that's unfortunate. If you could provide a debug log again, that'd be helpful!

grenzor commented 5 years ago

Log: https://send.firefox.com/download/f2a05e06e99a4588/#GqBShazevj6G8yf2lJtBqg

Process: middle clicked some links in succession from this github repo and checked the container name. 2 tabs opened with the same container name (tmp74) as the tab from which they were clicked from.

stoically commented 5 years ago

Perfect, that helped! Was able to spot an edge case where isolated mouseclicks are cleaned up while the request is still being handled. Fixed in v1.0beta7. Let me know if you encounter it again, thanks!

grenzor commented 5 years ago

@stoically I think the fix helped a bit but I still encounter the issue in 1.0beta7

Log: https://send.firefox.com/download/3eec8cc30ddf293b/#4ejsEBp6rMVYo13LLCiYUQ

Same process as before. Middle clicking a new tab opened it in container name tmp201 which is the same container as it was middle clicked from

stoically commented 5 years ago

Thanks. The mouse clicks occured in container id firefox-container-4267 (unfortunately can't see the tmp name) and according to the log all of them got correctly isolated. Container tmp201 has the container id firefox-container-4273 and was created due to an isolation. IIRC you have reuse container numbers active, correct? The log shows that some containers got removed, so in this case container numbers might've been freed, which might be the reason that you saw 2x tmp201?

grenzor commented 5 years ago

you have reuse container numbers active, correct?

I don't reuse the numbers. I'm using Keep Counting

stoically commented 5 years ago

Interesting. Then it seems like tmp201 got falsely reused. Should you encounter it again, would you mind checking the container list and see whether you have two containers with the same name?

grenzor commented 5 years ago

Sure. To make sure do you mean to check whether one container name (e.g. tmp201) has two container IDs? I also wiped the TC storage recently which reset the TC tmp numbers but I'm guessing TC can't reset the firefox-container-ID back to firefox-container-1 .

I do think the fix helped though as it does take longer before I encounter the issue. I'll keep testing. EDIT: Maybe not just encountered it again. Anyway will post a debug later

stoically commented 5 years ago

Checking about:preferences#containers to see whether the container name in question is listed twice would be enough. I'll try to add a test-case and see if I can reproduce it (because I can't in Firefox).

Yeah, the firefox-container- id is only resettable when reinstalling Firefox, TC can't modify them.

grenzor commented 5 years ago

Just encountered it and checked about:preferences#containers. The container name is listed once.

stoically commented 5 years ago

Meh. Well, another debug log would be helpful, because the last one you provided unfortunately didn't reveal any bug.

grenzor commented 5 years ago

Hopefully this log has something that will help https://send.firefox.com/download/01892f1d2c53db3f/#p1uyDLTfXaH3xWk5oGa0uA

Container tmp503 was the one which opened as the same container it was middle clicked from. Also checked again under about:preferences#containers and the name is only listed once

stoically commented 5 years ago

The link has expired, unfortunately.

grenzor commented 5 years ago

Log: https://send.firefox.com/download/ab393ec6c2988933/#72A551UIIS7ASFrb-vUSYA

stoically commented 5 years ago

There was indeed still something missing, fixed it and let you know when a new beta to test is available. Thanks!

stoically commented 5 years ago

v1.0beta15 is available, which hopefully fixes this bug for good. :D

grenzor commented 5 years ago

Thanks @stoically. I've got some good and bad news. Bad news first, I encountered the issue (unfortunately wasn't debugging then). Good news I encountered it only once and it is now very hard to reproduce the issue. The time I encountered it, it seemed like the middle clicked tabs took very long to load (not sure if related to TC or just my internet connection becoming slow)

I don't know if anything could be tweaked on TC side to make this occurrence less likely/more foolproof but the issue is now probably rare enough that I think it would be ok to consider this issue fixed. What do you think?

stoically commented 5 years ago

it seemed like the middle clicked tabs took very long to load

Yeah, that could be the cause - it's still possible that the delay between clicking a link and the tab starting to navigate is >1.5s, which results in the issue. The fixes applied until now only defer the delay with every encountered click, but if no new click occurs to defer, and the delay is >1.5s, it happens.

I don't know if anything could be tweaked on TC side to make this occurrence less likely/more foolproof

Well, actually, I just realized that there might be a much more cleaner solution to this.

Currently it goes like this:

link clicked
message to TC background
check if the clicked url should get isolated
store as "isolated mouseclick url"
register 1.5s cleanup for "isolated mouseclick url"
if a request comes in matching the "isolated mouseclick url" in 1.5s it gets isolated
every request after 1.5s is ignored

The cleanup is done to prevent isolation of unrelated requests to the same url, and that's where I probably missed the cleaner solution, it could maybe work like this:

link clicked
message to TC background
check if the clicked url should get isolated
store as "isolated mouseclick url + tabId where the click occured"
if request comes in, match against "isolated mouseclick url" and check whether the tabId or openerTabId match the "isolated mouseclick tabId"
only isolate if matches
clean up "isolated mouseclick" if all request handlers that matched the url+tabId finish (multiple mouse clicks, multiple running handlers)

This would allow to get rid of the timed clean up altogether - however, there are still edge-cases.

If an "isolated mouse click" is registered, but 1) the clicked link actually opens a different url (I'm looking at you js-link-trackers), there would no request occur that isolates and thus it wouldn't trigger cleanup 2) no main_frame request occurs, which could be the case for websites that use xhr to load new content (like github repos) or the tab started to navigate before the mouse click got registered (unlikely, never seen before), it wouldn't cleanup either

This is problematic because if the tab or openerTab then would try to navigate to the "isolated mouseclick url" any time later, it would result in an unexpected isolation. A possible cleanup solution for this might be: 1) if a tab starts to navigate whose tabId or openerTabId matches one of the "isolated mouse click tabIds", but with a different url, clean up that "isolated mouseclick" immediately 2) here we might actually need a timed cleanup - but that could be a larger one (like 1min or something), since

if the click results in a xhr request, it's unlikely that there ever will be an actual main_frame request to that url ever
since tabId and openerTabId are matched, it's unlikely that a request matching the "isolated mouseclick url" can come in, without being mouse clicked previously, because the mouse clicks isolation is still configured, though that means that "isolated mouse clicks" should be cleared if a mouse clicks isolation preference is changed

</braindump>

@grenzor Thanks for being persistent!

stoically commented 5 years ago

if the click results in a xhr request, it's unlikely that there ever will be an actual main_frame request to that url ever

This assumption is glaringly wrong - of course a main_frame request can occur in this case, simply by refreshing the tab.

it's unlikely that a request matching the "isolated mouseclick url" can come in, without being mouse clicked previously

Also wrong. If a mouseclick url in a tab is isolated and no request occurs as result of that, it's still possible that the originating tab starts navigating to the same url some time later without a mouseclick occurring, e.g. through redirects or some form of automation, and those requests would unexpectedly get isolated - unlikely, but totally possible.

It seems we're back full circle to timed cleanups.

That being said - I think there's potential to make the mouseclick isolation work better if they open new tabs, using webNavigation events. That would require to request a new permission that users have to confirm.

To avoid having a permission request pop up on upgrade for users, I'm going to make the permission optional, and put the Isolation preferences behind a "new permission needed"-button for existing users ("enable isolation"-button for new users). With that in place, I can actually make all permissions optional. I'll track that in #201.

With the webNavigation permission given, we can listen to onCreatedNavigationTarget events, which fire always really fast, since they're about the creation of the new tab, without any actual network request being involved (at least that's my understanding), unlike the currently used onBeforeRequest, which sometimes fires with a "huge" delay.

So, what needs to be done:

[x] Write more tests that cover https://github.com/stoically/temporary-containers/issues/277#issuecomment-507645557
[x] Refactor mouseclick isolation timed cleanup defer logic to be solid
[x] Improve mouseclick isolation behavior by working with tabId / openerTabId
[ ] Use onCreatedNavigationTarget to additionally defer mouseclick isolation cleanup

crssi commented 5 years ago

OT: Just wanted to thank you for all your hard work,,,, Thank you and cheers

stoically commented 5 years ago

While implementing the code around the onCreatedNavigationTarget event I realized that it doesn't actually help, because I can't use it to cancel the timed cleanup completely for the edge-case where a tab doesn't actually starts navigating (onBeforeRequest) to the url, which would mean the clicked url stays canceled for the involved tab ids - and deferring the timed cleanup also isn't helpful, because that's just some milliseconds from clicking the link until the event. Oh well.

However, I've implemented the fix for #277, the tab id handling mentioned in the earlier comments here and some additional logic that counts mouseclicks and only handles the clicked amount - published with v1.0beta20 - and hopefully that makes it really an exception to see mouse clicks not behaving as expected. The only thing remaining is when tabs have a delay from opening until navigation starts from over 1.5s, those won't get isolated - and from my current perspective there's nothing more I can do about it.

grenzor commented 5 years ago

Thanks for your effort on this @stoically. It does seem like a tricky thing to get rid of completely. I think as long as it's rare and doesn't happen regularly it should be good enough.

One probably dumb question I have is would it help to increase the 1.5s delay to something like 5s/longer or could that affect isolation in some ways?

EDIT: Also just tested out 1.0beta20 but it looks like https://github.com/stoically/temporary-containers/issues/277#issuecomment-508237517 still happens. Is that issue the same as the late navigation issue one here or a bug?

stoically commented 5 years ago

Increasing the timed cleanup would increase chances for unexpected isolations.

277 can still happen if the opened tab doesn't start loading (means: fires `onBeforeRequest`) before clicking the same link in the same tab, which unfortunately is in the same problem-scope as described in my earlier comment. If you want me to make sure that's the case, I'd be happy to look into a debug log.

grenzor commented 5 years ago

Seems similar, debug below just to make sure in case it's different

Log: https://send.firefox.com/download/a0ed039aeb33ed1c/#p0inFMyHtm2QlqBxoc3sHg

stoically commented 5 years ago

Thanks. So there was indeed still a bug. In your log the left-click request comes in first, so what should've happened is that the left-click gets isolated, decreases the mouseclick count to zero and deletes it because of that, and the middle click doesn't get isolated anymore because it already got deleted (handled). With the fix that happens now.

There might be a solution to not isolate the left click but the middle click instead in this case by taking the click type into account. But I'm not sure it's worth it - I mean, I'm curious, do you actually, like in day to day use, sometimes middle- and left-click the same link? If that's the case, I'll look into it. :]

grenzor commented 5 years ago

The first time I came across it was in my normal browsing before trying to reproduce it to get the debug log (edit: by the way I don't mean the one today, I mean https://github.com/stoically/temporary-containers/issues/277#issuecomment-508097796). Don't think I faced it normally since, though I'm not sure how frequently it would happen. It probably depends on the website I'm on. I'd say it's up to you, whether you think it's worth implementing the solution (and whether it would affect say a setup where the user wants left click to isolate).

stoically commented 5 years ago

Well, at this point I'll consider it an enhancement, not a bug anymore. So I'll look into it some time later with a fresh head - after the 1.0 release.

Here's the, hopefully final, v1.0beta21 which should isolate middle click correctly and don't isolate left-clicks done after that (if the middle-click request comes in first). Though, in both cases one click gets isolated, one doesn't - with the difference that the earlier left-click-request opens a new tab instead of navigating in the same tab.

Let me know if you experience any other issue - and thanks for thoroughly testing the beta and giving feedback!

grenzor commented 5 years ago

Thanks @stoically, happy to test :)

If it's possible, could you please say how the currently known issues will occur? Just so that I can post if I come across a new bug rather than something already known from the current issues scope.

For example when I middle click then left click in 1.0beta21 it still isolates both clicks (but is this currently expected because left click is somehow coming in first?) I guess I'm a little confused with what is currently expected due to the known issues and what is a not-expected bug.

stoically commented 5 years ago

It should only isolate one click either way - anything else would be a bug. What should happen:

middle and left click an url
middle click marks the url in question as isolated

Left click request comes in first

isolates request because of url and marks url as no longer isolated
middle click request comes in, doesn't get isolated because url is no longer marked as isolated

Middle click request comes in first

isolates request because of url and marks url as no longer isolated
left click request comes in, doesn't get isolated because url is no longer marked as isolated

grenzor commented 5 years ago

Thanks for explaining. Forget what I said about both clicks isolating, I do see only click being isolated. It was just the active tab which always showed as the one being isolated for me but the second tab wasn't isolated. Would it make sense to always make the non-isolated tab in this case the active tab or no?

stoically commented 5 years ago

I don't think that's possible with the current implementation, since that probably would mean detecting the correct mouse click isolation intents, which would solve the whole issue anyway. :D

stoically / temporary-containers

Mouse Click Isolation sometimes doesn't work as expected #184

Actual behavior

Expected behavior

Steps to reproduce