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

Associate userContextId=0 with default container (fix can't open in default container if url assigned to a container #1312) #1599

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

garywill
Copy link

@garywill garywill commented Dec 9, 2019

Background is #1312

"Open link in new container tab -> No container" doesn't work if the link is to the current site which has been set "always open in" current container

After debugging I found browser.tabs.onCreated is called twice.
The first call is ok. tab.cookieStoreId = firefox-default.
But the second call tab.cookieStoreId = firefox-container-x. The container id is not desired.

Then after digging the code, found in reloadPageInContainer() variable let currentCookieStoreId; is left undefined if currentUserContextId is false. That caused unwanted behavior.

@garywill garywill force-pushed the patch-1 branch 2 times, most recently from b8ad4e8 to 347a057 Compare December 9, 2019 09:22
@jonathanKingston
Copy link
Contributor

@garywill is this causing the code to do anything different or console errors?

My understanding is properties being undefined should use the firefox-default as that is how the extension APIs should work. (I would prefer we didn't pass undefined for the record, however I'm not sure if I prefer explicitly doing it either as this code might not account for future changes in Firefox either for example private browsing may eventually be supported).

@@ -490,6 +490,9 @@ const assignManager = {
currentCookieStoreId = backgroundLogic.cookieStoreId(currentUserContextId);
confirmUrl += `&currentCookieStoreId=${currentCookieStoreId}`;
}
else {
currentCookieStoreId = "firefox-default";
Copy link
Contributor

Choose a reason for hiding this comment

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

See my other comments however if we need this I would prefer all non test usage has a global.

@garywill
Copy link
Author

is this causing the code to do anything different or console errors

Yes it is causing browser doing different. No console warning or error is showed. And there's some odd behavior still confusing me.

If leave the variable undefined :

  • A tab with the comfirming page url is opened with tab.cookieStoreId = firefox-default. This seems ok.
  • After 0.5 sec, the tab closes automatically. Then immediatly another tab with the comfirming page url is opened with tab.cookieStoreId = firefox-container-x. (as I described above)

It's the second one that is undesired and giving user unwanted container. I have no idea why this happend. These behaviors can be observed by adding console.log in browser.tabs.onCreated.addListener (I did it on Firefox 68).

@garywill
Copy link
Author

garywill commented Feb 4, 2020

private browsing may eventually be supported

Some time ago this addon could be set as allowed in private window. But in private window user can't open any container tab, browser console shows Error: Illegal to set non-private cookieStoreId in a private window.

After "incognito": "not_allowed" was added in manifest.json, no longer this addon ran in private window. I don't see any sense to use this addon in a private window. Is there any plan to do that? Or is there any thing I miss?

@garywill garywill changed the title fix currentCookieStoreId left undefined #1312 fix currentCookieStoreId left undefined, can't open in default container if url assigned to a container #1312 Apr 10, 2020
associate userContextId=0 with CookieStoreId="firefox-default"
@garywill
Copy link
Author

garywill commented Mar 14, 2021

According to Firefox source code, default container has userContextId=0

https://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#776 in function createUserContextMenu()

 // If we are excluding a userContextId, we want to add a 'no-container' item.
  if (excludeUserContextId || showDefaultTab) {
    let menuitem = document.createXULElement("menuitem");
    menuitem.setAttribute("data-usercontextid", "0");
   .......

Addon's background logic didn't associate 0 with default container. userContextId should return 0 when cookieStoreId=firefox-default
Incognito mode don't have cookieStoreId, userContextId can be false

After changing, it seems no unnecessary reopening, so test fails. (Not that reason, sorry.) Now tests pass.

I also did some simple test (manually, with eyes) on some cases needing reopening. When opening example.com(which has been set "always open in container") in new tab, reopening works.

@garywill garywill changed the title fix currentCookieStoreId left undefined, can't open in default container if url assigned to a container #1312 Associate userContextId=0 with default container (fix can't open in default container if url assigned to a container #1312) Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants