When you're searching via the awesome bar, fx contacts the default search engine every time a key is pressed so it can attempt to autocomplete the users search term. It appears like a cookie is also being assigned. When you're searching via the awesomebar with different containers, the same cookie ID is being used for each key press/search. We should investigate to see if a search engine can pinpoint who the user is using these cookies despite being in different containers. I'm not really certain this is a bug, but definitely worth looking at. Example below: Banking Container: ================== 2016-01-29 22:15:35.946991 UTC - [0x100532790]: V/nsHttp uri=https://www.google.com/complete/search?client=firefox&q=apple 2016-01-29 22:15:35.946995 UTC - [0x100532790]: V/nsHttp nsHttpChannel::Init [this=114d54800] 2016-01-29 22:15:35.947126 UTC - [0x100532790]: V/nsHttp nsHttpChannel::AsyncOpen [this=114d54800] 2016-01-29 22:15:35.947148 UTC - [0x100532790]: V/nsHttp HttpBaseChannel::SetRequestHeader [this=114d54800 header="Cookie" value="NID=76=BiShhLGAWxRNAxo66asUuAHHdPFsedtn_z7Uf3g3vnz65C0VBiu7p36wCYzp8Mh9N5Q_khdiLTRe2vLw_1UmfGD-3vHzosJwJz5Fh9SICqO870FZn92cEqEOSWvWOE86" merge=0] Shopping Container: =================== 2016-01-29 22:15:49.299583 UTC - [0x100532790]: V/nsHttp uri=https://www.google.com/complete/search?client=firefox&q=android 2016-01-29 22:15:49.299590 UTC - [0x100532790]: V/nsHttp nsHttpChannel::Init [this=12d791800] 2016-01-29 22:15:49.299743 UTC - [0x100532790]: V/nsHttp nsHttpChannel::AsyncOpen [this=12d791800] 2016-01-29 22:15:49.299776 UTC - [0x100532790]: V/nsHttp HttpBaseChannel::SetRequestHeader [this=12d791800 header="Cookie" value="NID=76=BiShhLGAWxRNAxo66asUuAHHdPFsedtn_z7Uf3g3vnz65C0VBiu7p36wCYzp8Mh9N5Q_khdiLTRe2vLw_1UmfGD-3vHzosJwJz5Fh9SICqO870FZn92cEqEOSWvWOE86" merge=0] STR: (in the command terminal) * export NSPR_LOG_MODULES=timestamp,nsHttp:5,nsSocketTransport:5,nsStreamPump:5,nsHostResolver:5 * export NSPR_LOG_FILE=~/Desktop/log.txt * cd /Applications/FirefoxDeveloperEdition.app/Contents/MacOS * ./firefox-bin * Open the "Banking" container and search for something via the awesome bar * Open the "Shopping" container and search for something via the awesome bar * take a look at ~/Desktop/log.txt (you'll notice the same cookie ID is being used for both searches)

====================== |Google Search Engine| ====================== The cookies listed below were retreived using the "Live HTTP Headers" add-on and than double checked with the cookie manager under about:preferences#privacy Personal Container: ------------------- * domain=.google.com (initially searching via the awesome bar) ** Set-Cookie: NID=76=3vYx4sBFyZKktmY__MBQZl2PKAnsOjWxjogXQ5HSzgMhHuokHjBWainNtUZfMeJBfw82X74ys4BGw_YZRnxa2reSVAjlmx4tlV3xa2UuzxSkMxK-PI-xWhve6YgVXhlIuFtqzy7a; * domain=.google.ca; (redirected from .com to .ca) ** Set-Cookie: NID=76=UMq81m8R74hBRzIa3F3tvuiiLE-1Z2gipAWFxdAztQKDpDiW7swsXcvWqjd55G8tErNzz5VfiZf2X96HQiCgXqqZPHUBryC1LxLwVbpY7d-d5qV4Z2xrxs-4swHYx2_5; Work Container: --------------- * domain=.google.com (initially searching via the awesome bar) ** Set-Cookie: NID=76=Nhc9loSQd39MOAU03zb3OdzI7XGDaXTBHE3H9WQDK4Z7tYrOmwSu6UBpBxtOTEoV1jiyuv4WobG_58Rb0-bD_w8SP52aVWjgc0J7U6zPX0FnbafvgHS3C-d7mtqkuzCKkpXnZ1regw; * domain=.google.ca; (redirected from .com to .ca) ** Set-Cookie: NID=76=fY-A5OX_RjeXxZfxsSVwtHBja8XIUVmwxPS60_IxsYcDUsulbznQrxRdmXqSitFPt3qmsK4RcPsdDCWXgz1gD3qOnvHhD_B3lhRPJonq-f6d2M4fA8h9JYHZt3-uh8Bu; Banking Container: ------------------ * domain=.google.com (initially searching via the awesome bar) ** Set-Cookie: NID=76=JNwLReW0Zrg1hYZUakWLrehFrQWDsaEtiBjjpwYXBDxxHupj6GtiTeiWYCxelA58ilTkZHlR3jwvXjzQfj-c9NkkKVKak5e8xJhBBK3WLb9003YsUaj0ODvCdHoqHUtqLjFZkbxGPQ; * domain=.google.ca; (redirected from .com to .ca) ** Set-Cookie: NID=76=d0ZIzl3TnHDCPyADs0-UsAgCl8qzS0ab_a6P4ly3K6OWf-l0MK8Jq6Cg7qdACTVz49PGHBi476ZiReHELLKLFqNpZA0JCJZ_cKOsVqxAlTn_J6Iu2dehHa-kq-3eMNNN; Shopping Container: ------------------- * domain=.google.com (initially searching via the awesome bar) ** Set-Cookie: NID=76=BTj7nznAGkagwQz_JZYGw1xWHN35Z3g_xsgvXAFDpNTHzuvDiwKTDilCqHHOZtWZpzX7UV07PhwN5wmL0pODAsKNPsT5UgWNVFDN6_7aeAjtpbWRebxew_NC_GwbKA0X859inCFRIQ; * domain=.google.ca; (redirected from .com to .ca) ** Set-Cookie: NID=76=Y6GYO2rDxP1apat-xog2WNWy3LvqTh7qiMTmvJq2YZ7hJaIOTE5D7Qq8pc3VT_DFjMXb8tqV5bYXHkzK9KHktPi38jOlMz-OPzUYp8fm14BSE0s9bjwE7eNfutT9POlZ; ===================== |Yahoo Search Engine| ===================== Personal Container: ------------------- * Set-Cookie: B=6n8gtd1bbq1j1&b=3&s=a6; expires=Sun, 11-Feb-2018 22:08:33 GMT; path=/; domain=.yahoo.com * Set-Cookie: sB=nw=0&sh=1&pn=10&rw=&v=1; expires=Sat, 10-Feb-2018 22:08:33 GMT; path=/; domain=.search.yahoo.com * Set-Cookie: sSN=X4ZkD2M2wWFSaSmaw5Y0TpCf0bwHWjNCkjdDhlFwvz2iwvNjX2JKi8oD6f7g19J_FRtKeOz4UjuBuT7SOGlmuw--; path=/; domain=.search.yahoo.com Work Container: --------------- * Set-Cookie: B=73fu2d5bbq219&b=3&s=k8; expires=Sun, 11-Feb-2018 22:16:09 GMT; path=/; domain=.yahoo.com * Set-Cookie: sB=nw=0&sh=1&pn=10&rw=&v=1; expires=Sat, 10-Feb-2018 22:16:09 GMT; path=/; domain=.search.yahoo.com * Set-Cookie: sSN=dMsqX.M2wWGc1qMgWSuFAY6cuGvv25FkldzdOQvXDCeUIYJ7Yop3CyKJdz_8ejK8f1lIajf8DF2ykIwRTiWz0A--; path=/; domain=.search.yahoo.com Banking Container: ------------------ * Set-Cookie: B=4ij2c4lbbq29l&b=3&s=rd; expires=Sun, 11-Feb-2018 22:20:37 GMT; path=/; domain=.yahoo.com * Set-Cookie: sB=nw=0&sh=1&pn=10&rw=&v=1; expires=Sat, 10-Feb-2018 22:20:37 GMT; path=/; domain=.search.yahoo.com * Set-Cookie: sSN=360.PFo2wWFsFrK8MPBVQkwTJDeyIfAD7Dv8dbsnP2VqES4K8zML1zG3_1GTzHfbeaZ56qCLlAgXHqh3jpHULA--; path=/; domain=.search.yahoo.com Shopping Container: ------------------- * Set-Cookie: B=1sf8rl9bbq2g3&b=3&s=oh; expires=Sun, 11-Feb-2018 22:24:03 GMT; path=/; domain=.yahoo.com * Set-Cookie: sB=nw=0&sh=1&pn=10&rw=&v=1; expires=Sat, 10-Feb-2018 22:24:03 GMT; path=/; domain=.search.yahoo.com * Set-Cookie: sSN=wvTH7ew2wWFHjTJnUrzKcjy47Slz0tpdy6q5R4Zp4SfWyACyFdGc6w.qknIWrrSKpZWJuPYIdk6.RxRVUxq.LQ--; path=/; domain=.search.yahoo.com

I went through this in more detail, but this time using the Live HTTP Headers add-on rather than the NSPR_LOG_MODULES method which is really noisy and hard to parse. If you take a look at the results in comment #1, none of the containers are sharing the same cookies. I also tested to see if any cookies were being set when FX retrieves search suggestions from the search engine that's currently set as the default. Results below: Google: * doesn't set any cookies when fx looks for search suggestions ** fx's cookie manager was empty after going through all the containers Bing: * doesn't set any cookies when fx looks for search suggestions ** fx's cookie manager was empty after going through all the containers DuckDuckGo: * doesn't set any cookies when fx looks for search suggestions ** fx's cookie manager was empty after going through all the containers Yahoo: * Personal Container: (first container that retrieved the cookie) ** Set-Cookie: B=6j20ls9bbq32r&b=3&s=15; expires=Sun, 11-Feb-2018 22:34:03 GMT; path=/; domain=.yahoo.com * Work Container: (using the same cookie) ** Cookie: B=6j20ls9bbq32r&b=3&s=15 * Bank Container: (using the same cookie) ** Cookie: B=6j20ls9bbq32r&b=3&s=15 * Shopping Container: (using the same cookie) ** Cookie: B=6j20ls9bbq32r&b=3&s=15 * Default Container (None): (using the same cookie) ** Cookie: B=6j20ls9bbq32r&b=3&s=15 Two possible issues: Issue #1: The value of "Set-Cookie: sB=nw=0&sh=1&pn=10&rw=&v=1;" is constantly the same for every single container. From the looks of it, this value is static and never changes (not sure if it eventually changes as I keep getting the same value). I'm not sure what this value is used for, but could this be used pinpoint/track users? Even if they're using containers? Issue #2: (screenshot attached) As you can see from the results above, Yahoo sets a cookie whenever a user types something into any search fields and retrieves search suggestions from Yahoo. The cookie is placed into "Container: None" and all the other containers use the same cookie once it's set. I think this probably a leak we should address.

huseby, I believe you said one of your patches fixes this. Which bug?

So here's what's happening.. When you have Yahoo set as the default search engine, typing something into the awesome bar will create a cookie that belongs to the global context (Container: None) and has a blank entry under the "originAttributes" column in the cookies.sqlite database. This cookie is also used as the default cookie within the awesome bar/search bar for all the containers when FX retrieves search suggestions. The awesome bar/search bar is sharing the cookie that was created for the global context. I've attached a SQL dump from the cookies.sqlite database: * https://pastebin.mozilla.org/8860891 The first entry (the "B" cookie) was created when I typed something into the awesome bar via the userContextId=2. That same "B" cookie was used when I searched for something via Yahoo in the default container. You can see that a new "B" cookie wasn't created (id's 6-8 in the dump).

tanvi and kamil, yes, I'm pretty sure I have a patch that fixes this. I remember seeing the code related to the search context that wasn't passing the origin attributes along. This, however may be a bug caused by the cookie enumerator not knowing about user contexts. It's curious to me that this is broken when cookie lookup and send works in XULBrowsers in general. Hrm...Let me see if I can find the patch. I'm going to leave the ni? for me so I don't forget.

Using Yahoo as the default search engine, the awesome bar uses all the cookies that have been set for the global context in every container. In this case, I logged into Yahoo using the default container and recorded the cookies that were created. In a personal container, I started typing a search term into the awesome bar, which contacted Yahoo and sent most of the cookies that were created previously in the default container despite being in a personal container that should be compartmentalizing cookies. Cookies that were created when logging into Yahoo via the default container: ==================== Set-Cookie: AO=u=1; expires=Sun, 02-Mar-2036 12:38:02 GMT; path=/; domain=.yahoo.com Set-Cookie: B=7is8ondbde4ps&b=4&d=FcRNLVNpYFRw_vR4iCWDLcJQb0Q-&s=9m&i=uj1ySYXVcmXblCrMTf1Q; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.yahoo.com Set-Cookie: F=a=pcaYblQMvS5IVjIkPcfgFoxJL6U2Xzy02jm_b89JGkotYz0zOy6_uI4pFlsmcs85NQlFGI4-&b=97DQ&d=eEaD_HM9vK8T3FGVdlM2_ic6QJoZkGM4lPw5fAZG.7j9; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.yahoo.com; HttpOnly Set-Cookie: FS=v=0&d=o0avnGWoaX4ZunIeqdJjabikkZu7HAYThdrrwek8Hw--; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.login.yahoo.com; secure; HttpOnly Set-Cookie: PH=fn=IGJnmMAleXBHIESeHD8-&l=en-CA&i=ca; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.yahoo.com Set-Cookie: SSL=v=1&s=OXqiALtjdVq2CImdu8m7VW1BsWIRWGjNNLsm1.nhJPY8PBo_nl06u4wSfiJhgbmqkZ1_auV6Q21OddLJJ4thvg--&kv=0; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.yahoo.com; secure; httponly Set-Cookie: T=z=SNx1WBShY6WBOhwTL5.JYdGNk8yNwY2MzE0MTA3NzYzTk5OMj&a=QAE&sk=DAAyxKlnQubS1e&ks=EAAk6Q60kuCU4BJXvl245P_rA--~E&d=c2wBTVRnMU1BRXhORFl6Tmpjd01ERTBPVGs1TlRjNE9BLS0BYQFRQUUBZwFKUlROR1hDU0E2U0ZRNEhJNFdRMk5IREhPRQFzY2lkATdPWUluY0xmUjF1VkliNWticlg2U3QuSXd6by0BYWMBQU1RRUlhbHIBdGlwAXVINVBXQQFzYwFkZXNrdG9wX3dlYgFmcwFTWFBuazR4VzF4TlMBenoBU054MVdCQTdF; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.yahoo.com; HttpOnly Set-Cookie: Y=v=1&n=0cmgac1iuv9tk&l=a0c8b9epm80ar/o&p=m2lvvca012000000&iz=&r=102&lg=en-CA&intl=ca; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.yahoo.com Set-Cookie: YLS=v=1&p=1&n=1; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.yahoo.com Set-Cookie: YP=v=AwAAY&d=AEcAMEQCIAkQ8ofkN47Fsq5skFG1xssRUI5w5mWf9.7tMI4zvppkAiBPaqHHn0tgVghE6I9MBzvjTijrBagBE8xAtUd4n9vG_AA-; expires=Sat, 03-Mar-2018 16:22:42 GMT; path=/; domain=.yahoo.com; secure; HttpOnly ==================== The http header that's being sent when using the awesome bar via other containers.. Notice it's using the same cookies that were set in the default container as mentioned in the example above: ==================== GET /sugg/ff?output=fxjson&appid=ffd&command=kami HTTP/1.1 Host: search.yahoo.com User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Accept-Language: en-US,en;q=0.5 Accept-Encoding: gzip, deflate, br Cookie: B=7is8ondbde4ps&b=4&d=FcRNLVNpYFRw_vR4iCWDLcJQb0Q-&s=9m&i=uj1ySYXVcmXblCrMTf1Q; AO=u=1; F=a=pcaYblQMvS5IVjIkPcfgFoxJL6U2Xzy02jm_b89JGkotYz0zOy6_uI4pFlsmcs85NQlFGI4-&b=97DQ&d=eEaD_HM9vK8T3FGVdlM2_ic6QJoZkGM4lPw5fAZG.7j9; PH=fn=IGJnmMAleXBHIESeHD8-&l=en-CA&i=ca; SSL=v=1&s=OXqiALtjdVq2CImdu8m7VW1BsWIRWGjNNLsm1.nhJPY8PBo_nl06u4wSfiJhgbmqkZ1_auV6Q21OddLJJ4thvg--&kv=0; T=z=SNx1WBShY6WBOhwTL5.JYdGNk8yNwY2MzE0MTA3NzYzTk5OMj&a=QAE&sk=DAAyxKlnQubS1e&ks=EAAk6Q60kuCU4BJXvl245P_rA--~E&d=c2wBTVRnMU1BRXhORFl6Tmpjd01ERTBPVGs1TlRjNE9BLS0BYQFRQUUBZwFKUlROR1hDU0E2U0ZRNEhJNFdRMk5IREhPRQFzY2lkATdPWUluY0xmUjF1VkliNWticlg2U3QuSXd6by0BYWMBQU1RRUlhbHIBdGlwAXVINVBXQQFzYwFkZXNrdG9wX3dlYgFmcwFTWFBuazR4VzF4TlMBenoBU054MVdCQTdF; Y=v=1&n=0cmgac1iuv9tk&l=a0c8b9epm80ar/o&p=m2lvvca012000000&iz=&r=102&lg=en-CA&intl=ca; YLS=v=1&p=1&n=1; YP=v=AwAAY&d=AEcAMEQCIAkQ8ofkN47Fsq5skFG1xssRUI5w5mWf9.7tMI4zvppkAiBPaqHHn0tgVghE6I9MBzvjTijrBagBE8xAtUd4n9vG_AA- Connection: keep-alive ==================== I went through the same case using private browsing and logged into Yahoo which created new cookies as expected: ==================== Set-Cookie: AO=u=1; expires=Sun, 02-Mar-2036 12:41:34 GMT; path=/; domain=.yahoo.com Set-Cookie: B=04scno1bde50e&b=4&d=FcRNLVNpYFRw_vR4iCWDLcJQb0Q-&s=cd&i=o5IJlBcMgcWIEOeMNWbu; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.yahoo.com Set-Cookie: F=a=Pd8UnD0MvS7vAyEVLD8IpCZ9Z9q1.6uEN9WbR9mfiL7jJPNjgIEmdsW9AUblvEKYvF1VNV8-&b=LZbU&d=eEaD_HM9vK8T3FGVdlM2_ic6QJoZkGM4lPw5fAZG.7j9; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.yahoo.com; HttpOnly Set-Cookie: FS=v=0&d=EOwj2_ObS77HqAqE5uy55SgAFM09WQylysZZZlXqww--; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.login.yahoo.com; secure; HttpOnly Set-Cookie: PH=fn=Ij09gY5RfnB8YJKm0hc-&l=en-CA&i=ca; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.yahoo.com Set-Cookie: SSL=v=1&s=HQteQSwpBnpPM2O2eKhli9KcaxZj7rPlZwHvIejRGoKInIyv7reuPqldF_kPytNBgQngC8ag82qZL8vJ_1enMw--&kv=0; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.yahoo.com; secure; httponly Set-Cookie: T=z=mQx1WBmkY6WBnmq/g4zigFUNk8yNwY2MzE0MTA3NzYzTk5OMj&a=QAE&sk=DAAoSYHnzYpxS3&ks=EAAjkAsoPL21ZQvaxPlutBF4A--~E&d=c2wBTVRnMU1BRXhORFl6Tmpjd01ERTBPVGs1TlRjNE9BLS0BYQFRQUUBZwFKUlROR1hDU0E2U0ZRNEhJNFdRMk5IREhPRQFzY2lkAXM4Ll9vUTk2aGVLTWVCUDVndncyaWI3UDJvdy0BYWMBQU0xemFiVkoBdGlwAXVINVBXQQFzYwFkZXNrdG9wX3dlYgFmcwFlbEVuRXh0VzF4UW0BenoBbVF4MVdCQTdF; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.yahoo.com; HttpOnly Set-Cookie: Y=v=1&n=0cmgac1iuv9tk&l=a0c8b9epm80ar/o&p=m2lvvca012000000&iz=&r=102&lg=en-CA&intl=ca; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.yahoo.com Set-Cookie: YLS=v=1&p=1&n=1; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.yahoo.com Set-Cookie: YP=v=AwAAY&d=AEkAMEYCIQCTsFapaQdlLmBUhepjHez8HHQx4.cqGxA42KpmNw_mzgIhANz.tXd5UiVCWq.e3URHnj6L6470cyDi5_Ep6UjM3iqwAA--; expires=Sat, 03-Mar-2018 16:26:14 GMT; path=/; domain=.yahoo.com; secure; HttpOnly ==================== Private browsing doesn't seem to provide search terms when typing into the awesome bar which means it's not contacting Yahoo as much as non-private browsing windows. However, the awesome bar does manage to contact Yahoo via the awesome bar from time to time, sending the following header with all the cookies: ==================== https://comet.yahoo.com/comet POST /comet HTTP/1.1 Host: comet.yahoo.com User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0 Accept: */* Accept-Language: en-US,en;q=0.5 Accept-Encoding: gzip, deflate, br Referer: https://www.yahoo.com/ X-Requested-With: XMLHttpRequest Content-Type: application/json Authorization: Basic token= Content-Length: 244 Origin: https://www.yahoo.com Cookie: B=04scno1bde50e&b=4&d=FcRNLVNpYFRw_vR4iCWDLcJQb0Q-&s=cd&i=o5IJlBcMgcWIEOeMNWbu; ucs=lnct=1456935952; AO=u=1; F=a=Pd8UnD0MvS7vAyEVLD8IpCZ9Z9q1.6uEN9WbR9mfiL7jJPNjgIEmdsW9AUblvEKYvF1VNV8-&b=LZbU&d=eEaD_HM9vK8T3FGVdlM2_ic6QJoZkGM4lPw5fAZG.7j9; PH=fn=Ij09gY5RfnB8YJKm0hc-&l=en-CA&i=ca; SSL=v=1&s=HQteQSwpBnpPM2O2eKhli9KcaxZj7rPlZwHvIejRGoKInIyv7reuPqldF_kPytNBgQngC8ag82qZL8vJ_1enMw--&kv=0; T=z=mQx1WBmkY6WBnmq/g4zigFUNk8yNwY2MzE0MTA3NzYzTk5OMj&a=QAE&sk=DAAoSYHnzYpxS3&ks=EAAjkAsoPL21ZQvaxPlutBF4A--~E&d=c2wBTVRnMU1BRXhORFl6Tmpjd01ERTBPVGs1TlRjNE9BLS0BYQFRQUUBZwFKUlROR1hDU0E2U0ZRNEhJNFdRMk5IREhPRQFzY2lkAXM4Ll9vUTk2aGVLTWVCUDVndncyaWI3UDJvdy0BYWMBQU0xemFiVkoBdGlwAXVINVBXQQFzYwFkZXNrdG9wX3dlYgFmcwFlbEVuRXh0VzF4UW0BenoBbVF4MVdCQTdF; Y=v=1&n=0cmgac1iuv9tk&l=a0c8b9epm80ar/o&p=m2lvvca012000000&iz=&r=102&lg=en-CA&intl=ca; YLS=v=1&p=1&n=1; YP=v=AwAAY&d=AEkAMEYCIQCTsFapaQdlLmBUhepjHez8HHQx4.cqGxA42KpmNw_mzgIhANz.tXd5UiVCWq.e3URHnj6L6470cyDi5_Ep6UjM3iqwAA-- Connection: keep-alive [{"id":"16","ext":{"timestamp":1456936029118,"host":"www.yahoo.com"},"channel":"/meta/connect","connectionType":"long-polling","clientId":"bf1onepush69.24691321456935977515191500056267.NHHokjO.gx2WOzuLrPSzljUIbKKkMDuddlHAS_TCem71fv9Lik6Q7xmS"}] HTTP/1.1 200 OK Access-Control-Allow-Origin: https://www.yahoo.com Access-Control-Allow-Credentials: true Content-Type: application/json Date: Wed, 02 Mar 2016 16:27:30 GMT Age: 21 Transfer-Encoding: chunked Connection: keep-alive Server: ATS/5.0.1 ====================

Hi Kamil, Can you confirm your comment 6 is accurate? I recall you revisited this and found different cookies were sent for the awesomebar searches.

Tanvi, I went through this again using the Live HTTP Headers add-on [1], results [2]: When typing anything into the awesome bar while Yahoo is the default search engine, the cookie from the default container is being used while FX pulls the search terms and populates them into the awesome bar. This affects all the containers. Once you actually search for something via a container, a new B cookie is created and assigned to that container. However, typing into the awesome bar will always use the default container cookie until the user actually initiates a search. * typing anything into the awesome bar will use the default cookie to pull search terms even though you're in a different container * initiating a search using the awesome bar will correctly use a different cookie for each container This doesn't affect Private Browsing mode. PB uses it's own cookies and never shares any cookies between the other containers. [1] https://addons.mozilla.org/en-US/firefox/addon/live-http-headers/ [2] https://pastebin.mozilla.org/8869538

The awesomebar should respect usercontextid. So if a user is logged into a search provider as follows: * Logged in with Account1 in the Default Context * Logged in with Account2 in the Work Context * Not logged in with the Shopping Context, but has some non-login cookies sent. When that user uses the awesomebar to search, only the cookies associated with the usercontextID of the current tab should be sent. So cookies for Account1 shouldn't be sent in the Shopping Context. This bug is high priority, as it causes leakage across contexts. I'd like to see this picked up and fixed for Firefox 49.

Kamil recorded the STR and it's quite helpful: https://www.youtube.com/watch?v=ynkr02Qce0g&feature=youtu.be

The autocompletion from Awesomebar is running from XUL, not from the container. That's why when the search engine queries the result, it will use the cookies from default container. Also the query is using XHR with SystemPrincipal, so there's no origin attributes with that channel. The caller is from https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/SearchSuggestionController.jsm#252 I'd like to ask Tanvi's opinion on this. Since I don't know if making the query running under tab instead of XUL makes sense, or should we disallow sending cookie in this case? below is the call stack (cpp and JS) Breakpoint 1, mozilla::net::HttpBaseChannel::SetRequestHeader (this=0x7fffce411000, aHeader=..., aValue=..., aMerge=false) at /home/allstars/ssd/gecko-dev/netwerk/protocol/http/HttpBaseChannel.cpp:1572 1572 printf("XXX Found Search\n"); (gdb) bt #0 0x00007fffe3cae0a7 in mozilla::net::HttpBaseChannel::SetRequestHeader(nsACString_internal const&, nsACString_internal const&, bool) (this=0x7fffce411000, aHeader=..., aValue=..., aMerge=false) at /home/allstars/ssd/gecko-dev/netwerk/protocol/http/HttpBaseChannel.cpp:1572 #1 0x00007fffe3cb2880 in mozilla::net::HttpBaseChannel::AddCookiesToRequest() (this=0x7fffce411000) at /home/allstars/ssd/gecko-dev/netwerk/protocol/http/HttpBaseChannel.cpp:2680 #2 0x00007fffe3d2bcdb in mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*) (this=0x7fffce411000, listener=0x7fffc2d7a190, context=0x0) at /home/allstars/ssd/gecko-dev/netwerk/protocol/http/nsHttpChannel.cpp:5180 #3 0x00007fffe3d2bfea in mozilla::net::nsHttpChannel::AsyncOpen2(nsIStreamListener*) (this=0x7fffce411000, aListener=0x7fffc2d7a190) at /home/allstars/ssd/gecko-dev/netwerk/protocol/http/nsHttpChannel.cpp:5229 #4 0x00007fffe53253e6 in nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&) (this=0x7fffc3d47400, aVariant=0x0, aBody=...) at /home/allstars/ssd/gecko-dev/dom/base/nsXMLHttpRequest.cpp:2858 #5 0x00007fffe5daf403 in nsXMLHttpRequest::Send(mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&) (this=0x7fffc3d47400, aBody=...) at /home/allstars/ssd/gecko-dev/dom/base/nsXMLHttpRequest.h:419 #6 0x00007fffe5daf4ba in nsXMLHttpRequest::Send(JSContext*, mozilla::ErrorResult&) (this=0x7fffc3d47400, aRv=...) at /home/allstars/ssd/gecko-dev/dom/base/nsXMLHttpRequest.h:437 #7 0x00007fffe5daf70c in nsXMLHttpRequest::Send(JSContext*, nsAString_internal const&, mozilla::ErrorResult&) (this=0x7fffc3d47400, aCx=0x7fffd9f05400, aString=..., aRv=...) at /home/allstars/ssd/gecko-dev/dom/base/nsXMLHttpRequest.h:462 ... (gdb) call DumpJSStack() 0 this.SearchSuggestionController.prototype._fetchRemote(searchTerm = "worr", engine = [xpconnect wrapped nsISearchEngine @ 0x7fffcf65cda0 (native @ 0x7fffc5623e80)], privateMode = false) ["resource://gre/modules/SearchSuggestionController.jsm":255] this = [object Object] 1 this.SearchSuggestionController.prototype.fetch(searchTerm = "worr", privateMode = false, engine = [xpconnect wrapped nsISearchEngine @ 0x7fffcf65cda0 (native @ 0x7fffc5623e80)]) ["resource://gre/modules/SearchSuggestionController.jsm":142] this = [object Object] 2 SearchSuggestionControllerWrapper(engine = [xpconnect wrapped nsISearchEngine @ 0x7fffcf65c4a0 (native @ 0x7fffc5623e80)], searchToken = "worr", inPrivateContext = false, maxResults = 10) ["resource://gre/modules/PlacesSearchAutocompleteProvider.jsm":134] this = [object Object] 3 getSuggestionController(searchToken = "worr", inPrivateContext = false, maxResults = 10) ["resource://gre/modules/PlacesSearchAutocompleteProvider.jsm":121] this = [object Object] 4 getSuggestionController(searchToken = "worr", inPrivateContext = false, maxResults = 10) ["resource://gre/modules/PlacesSearchAutocompleteProvider.jsm":290] this = [object Object] 5 _matchSearchSuggestions() ["file:///home/allstars/ssd/gecko-dev/obj-x86_64-pc-linux-gnu/dist/bin/components/UnifiedComplete.js":1019] 6 InterpretGeneratorResume(gen = [object Generator], val = undefined, kind = "next") ["self-hosted":1189] 7 next(val = undefined) ["self-hosted":1096] this = [object Generator] 8 TaskImpl_run(aSendResolved = true) ["resource://gre/modules/Task.jsm":319] this = [object Object] 9 TaskImpl(iterator = [object Generator]) ["resource://gre/modules/Task.jsm":280] this = [object Object] 10 createAsyncFunction/asyncFunction() ["resource://gre/modules/Task.jsm":254] 11 Task_spawn(aTask = [object Generator]) ["resource://gre/modules/Task.jsm":168] this = [object Object] 12 TaskImpl_handleResultValue(aValue = [object Generator]) ["resource://gre/modules/Task.jsm":388] this = [object Object](gdb) c

(In reply to Yoshi Huang[:allstars.chh] from comment #11) > The autocompletion from Awesomebar is running from XUL, not from the > container. > That's why when the search engine queries the result, it will use the > cookies from default container. > Also the query is using XHR with SystemPrincipal, so there's no origin > attributes with that channel. > > The caller is from > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/ > SearchSuggestionController.jsm#252 > > I'd like to ask Tanvi's opinion on this. > Since I don't know if making the query running under tab instead of XUL > makes sense, > or should we disallow sending cookie in this case? This looks like a hard problem. We need to send cookies, but we want to make sure we are sending them for the right context. Is there anyway to get the usercontextId here? Since loadInfo->loadingPrincipal is system Principal, we can't it get from there. Can we get it from tabbrowser.xml? The other alternative is adding more loadInfo arguments, but that is also very hard.

CC'ing both Kev Needham and Mike Connor as I believe they're still the contacts for anything Yahoo Search related. Maybe they'll some input about this issue.

Hi Gijs When we try to type something in URL bar(Awesome bar), Gecko will send some query (by XHR) to Search Engine to get search suggestions back. However the search suggestions is running on XUL, which means the cookie of default userContextId will be used, even though now we are in a container tab with non-default userContextId. I've asked Tanvi and Jonas about this, they both think we should find a way to pass the origin attributes to search suggestions and can be able to choose the cookie jar we want to use, since it's running with System Principal. I have a some draft patch about passing some userContext information to search suggestions, and it is done base on how we pass the information when we are in Private Browsing. I know I still have lots things to do in this bug, but I'd like to request for your feedback first, can you help to give me some suggestions here. Thanks in advance.

Comment on attachment 8750744 [details] [diff] [review] WIP - Patch, Pass UserContext to Search Suggestions. Review of attachment 8750744 [details] [diff] [review]: ----------------------------------------------------------------- I don't know unifiedcomplete or autocomplete well enough to give good feedback here, but I tried to give you some... I flagged Marco instead / in addition, to see if we're missing something simple. How do we currently deal with searches in private browsing mode? It looks like they're just disabled... that doesn't seem like the right thing to do either? ::: browser/base/content/urlbarBindings.xml @@ +892,5 @@ > this._value = this.inputField.value; > gBrowser.userTypedValue = this.value; > + > + if (gBrowser.selectedTab.hasAttribute("usercontextid")) { > + let userContextId = gBrowser.selectedTab.getAttribute("usercontextid"); It feels like this should ask the selected browser for the property in question instead. @@ +894,5 @@ > + > + if (gBrowser.selectedTab.hasAttribute("usercontextid")) { > + let userContextId = gBrowser.selectedTab.getAttribute("usercontextid"); > + //TODO: pass userContextId. > + this.searchParam += " user-context"; This is wrong - it'll just keep adding "user-context" to searchParam over and over again. We should also not need to adjust this for every keypress / input operation in the url bar - the user context id doesn't change after a tab is opened, right? It feels like we should adjust this somewhere else. ::: toolkit/components/search/SearchSuggestionController.jsm @@ +104,5 @@ > * results from them will not be provided. > * > * @param {string} searchTerm - the term to provide suggestions for > * @param {bool} privateMode - whether the request is being made in the context of private browsing > + * @param {bool} userContext - TODO: This can't be a bool, right? userContextId is an int... We'll need to parse it out somehow. @@ +235,2 @@ > let deferredResponse = Promise.defer(); > + //TODO pass userContext to xhr Is there actually an API we can use for this, or do we need to stop using XHR? That would be a big, kind of scary change...

Are these cookies actually useful for the user? couldn't we make the connection refuse any cookie for these requests?

(In reply to :Gijs Kruitbosch from comment #15) > How do we currently deal with searches in private browsing mode? It looks > like they're just disabled... that doesn't seem like the right thing to do > either? > Yeah, it is disable in private browsing. Bug 1182792 > ::: browser/base/content/urlbarBindings.xml > @@ +892,5 @@ > > this._value = this.inputField.value; > > gBrowser.userTypedValue = this.value; > > + > > + if (gBrowser.selectedTab.hasAttribute("usercontextid")) { > > + let userContextId = gBrowser.selectedTab.getAttribute("usercontextid"); > > It feels like this should ask the selected browser for the property in > question instead. > Okay, will do. > @@ +894,5 @@ > > + > > + if (gBrowser.selectedTab.hasAttribute("usercontextid")) { > > + let userContextId = gBrowser.selectedTab.getAttribute("usercontextid"); > > + //TODO: pass userContextId. > > + this.searchParam += " user-context"; > > This is wrong - it'll just keep adding "user-context" to searchParam over > and over again. We should also not need to adjust this for every keypress / > input operation in the url bar - the user context id doesn't change after a > tab is opened, right? It feels like we should adjust this somewhere else. > Okay, will fix this. > ::: toolkit/components/search/SearchSuggestionController.jsm > @@ +104,5 @@ > > * results from them will not be provided. > > * > > * @param {string} searchTerm - the term to provide suggestions for > > * @param {bool} privateMode - whether the request is being made in the context of private browsing > > + * @param {bool} userContext - TODO: > > This can't be a bool, right? userContextId is an int... We'll need to parse > it out somehow. > Yeah, I am considering using a JS dict, but use bool for simplicity now. Because the value is passed from the 'searchParam' in nsIAutoCompieteInput, which is a String. I am not sure if passing "enable-actions userContextId:1" in searchParam makes sense or not. > @@ +235,2 @@ > > let deferredResponse = Promise.defer(); > > + //TODO pass userContext to xhr > > Is there actually an API we can use for this, or do we need to stop using > XHR? That would be a big, kind of scary change... Yeah, my next step is to create another interface for XHR to specify origin attributes. I agree it's a huge hack. :P But from Jonas' point, (If I understand him point correctly), System Principal consumes all princinpals, so we could make it to choose the data-jar we want to use. I think the same idea is used for SafeBrowsing. The principal is also System Principal, but we add another SAFE_BROWSING_APP_ID to let it has its own cookie jar. (In reply to Marco Bonardo [::mak] from comment #16) > Are these cookies actually useful for the user? couldn't we make theu From Tanvi's point, she said we need to use cookie in the search suggestions, that's from the requirement of the contract with search engine. But I don't know the details.

Hi Jonas I tried to override the OA in XHR.channel.loadInfo and it seems working. So I'd like to request for feedback from you to see if this is the correct way to do this, and also you are the reviewer of Bug 1175685. (overriding OriginAttributes in SafeBrowsing). Thanks

Comment on attachment 8750744 [details] [diff] [review] WIP - Patch, Pass UserContext to Search Suggestions. Review of attachment 8750744 [details] [diff] [review]: ----------------------------------------------------------------- Using searchParams is probably the only way to pass through something from the view to the model, so that's fine, I don't think we have other solutions, you could use a special char to separate the name from the value. Gijs feedback is pretty much underlining all the issues already, the param should ideally not be set on input since it doesn't change that often, maybe on tab switch if the context id is per tab? Or we could use the unique id of the current browser to set a _contextIdSetByBrowser and replaces the searchparam value if the id differs. In any case there should only be one contextid set at a time, so if there's one already it should be replaced.

Comment on attachment 8751203 [details] [diff] [review] WIP - Part 2: override OriginAttributes in XHR Review of attachment 8751203 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsNetUtil.cpp @@ -1265,5 @@ > - } > - > - DocShellOriginAttributes doa; > - loadContext->GetOriginAttributes(doa); > - aAttributes.InheritFromDocShellToNecko(doa); This will affect a lot of existing code. It's definitely not something we want to do in this bug. It needs to be done much more carefully. Instead make the XHR object create a temporary LoadContext and set the appropriate attributes on that LoadContext. ::: toolkit/components/search/SearchSuggestionController.jsm @@ +241,5 @@ > this._request.open(method, submission.uri.spec, true); > + > + //TODO pass correct userContextId to xhr > + if (userContext) { > + this._request.channel.loadInfo.originAttributes = {userContextId: 1}; Please don't use XMLHttpRequest.channel. It's an API that we really should deprecate and is causing quite a lot of pain inside of the XHR implementation. Please add a XMLHttpRequest.setOriginAttributes(OA) function instead. [ChromeOnly] of course. This function should both affect the LoadContext that is used by the XHR object, *and* the LoadInfo. Otherwise assertions will fire that indicate that LoadInfo and LoadContext are not in sync. In fact, I'm surprised those assertions didn't fire with this patch. Did you test the patch in debug builds?

(In reply to Jonas Sicking (:sicking) from comment #20) > In fact, I'm surprised those assertions didn't fire with this patch. Did you > test the patch in debug builds? Yes, I am about to mention this and checking it today. It did have a lot of assetions with mismatch information between loadContext and loadinfo. But my original intention to ask feedback from you to see overriding the OA in loadinfo is the right way to go, as I thought it would be much like of safebrowsing. Thanks for your valuable comments, I'll do as you suggested, and will have full try run first next time.

For now we need to keep LoadInfo and LoadContext in sync. Ideally we should deprecate LoadContext, but lots of work remaining before we can do that. Btw, no need to do full try run, at least not until you're asking for review. But some testing in debug builds is always a good idea.

(In reply to Jonas Sicking (:sicking) from comment #20) > Please add a XMLHttpRequest.setOriginAttributes(OA) function instead. > [ChromeOnly] of course. > Hi Jonas At first I thought you said 'ChromeOnly' by mistake, because the caller here (SearchSuggestionController.jsm) is using Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest); Thus I should modify the nsIXMLHttpRequest.idl, which doesn't have ChromeOnly annotation. But it turns out you're right, I have to modify XMLHttpRequest.webidl. Otherwise the function will be undefined if I added it in IDL. https://gist.github.com/allstarschh/74a2f436f690a7349311317171d10f54 But why? Shouldn't it be IDL in this case?

(In reply to Yoshi Huang[:allstars.chh] from comment #23) > (In reply to Jonas Sicking (:sicking) from comment #20) > > Please add a XMLHttpRequest.setOriginAttributes(OA) function instead. > > [ChromeOnly] of course. > > > Hi Jonas > At first I thought you said 'ChromeOnly' by mistake, because the caller here > (SearchSuggestionController.jsm) is using > Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci. > nsIXMLHttpRequest); > > Thus I should modify the nsIXMLHttpRequest.idl, which doesn't have > ChromeOnly annotation. > > But it turns out you're right, I have to modify XMLHttpRequest.webidl. > Otherwise the function will be undefined if I added it in IDL. > https://gist.github.com/allstarschh/74a2f436f690a7349311317171d10f54 > > But why? Shouldn't it be IDL in this case? Because of this https://bugzilla.mozilla.org/show_bug.cgi?id=780529#c2 ?

Objects that expose .webidl interfaces are always scriptable only through webidl. XPConnect will not be invoked at all for such objects, and thus any .idl interfaces that the object implements doesn't affect what script see at all.

Comment on attachment 8756690 [details] [diff] [review] WIP Patch Review of attachment 8756690 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsXMLHttpRequest.cpp @@ +1680,5 @@ > + NeckoOriginAttributes neckoAttrs; > + > + //TODO: how to convert to JS::Handle<JS::Value> > + attrs.Init(aCx, aAttrs); > + neckoAttrs.Init(aCx, aAttrs); I am not sure how to do the conversion here? or move the cstor of OriginAttributes to public? Bobby, can you give me some suggestions?

Comment on attachment 8756690 [details] [diff] [review] WIP Patch Review of attachment 8756690 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, about to go on PTO and I don't have time to look at this.

Comment on attachment 8757839 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest Review of attachment 8757839 [details] [diff] [review]: ----------------------------------------------------------------- I am still trying to have a test for this. However ask feedback from Jonas first.

Comment on attachment 8757839 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest Review of attachment 8757839 [details] [diff] [review]: ----------------------------------------------------------------- I am still trying to have a test for this. However ask feedback from Jonas first.

Hi Gijs and mak I've modified my patch base on your previous suggestions, can you take a look on this again and give me some feedback ? I am still working on tests, if you think there are some other cases should be tested, feel free to comment :D Thanks

Comment on attachment 8758330 [details] [diff] [review] WIP Part 3: pass userContextId to autocomplete/search suggestion Review of attachment 8758330 [details] [diff] [review]: ----------------------------------------------------------------- This is a cursory review, but here are some notes from me. I'll let Marco comment on the autocomplete controller changes. ::: toolkit/components/places/UnifiedComplete.js @@ +680,5 @@ > this._inPrivateWindow = params.has("private-window"); > this._prohibitAutoFill = params.has("prohibit-autofill"); > > + let userContext = searchParam.match(/userContextId:\d/); > + this._userContextId = userContext ? userContext[0].split(":")[1] : 0; This will result in a string sometimes, and a number at other times. We should probably ensure it's a number. It also only deals with single digit numbers and could be simplified by using groupings in the regexp. I would instead do: let userContextId = searchParam.match(/(?:^| )userContextId:(\d+)); this._userContextId = userContextId ? 1 * userContextId[1] : 0; ::: toolkit/components/search/SearchSuggestionController.jsm @@ +240,5 @@ > let method = (submission.postData ? "POST" : "GET"); > this._request.open(method, submission.uri.spec, true); > + > + if (userContextId != Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID) { > + this._request.setOriginAttributes({userContextId}); This overwrites the other origin attributes unless/until bug 1274689 changes that. You're certainly removing the private mode setter, which also seems wrong... ::: toolkit/content/widgets/autocomplete.xml @@ +625,5 @@ > > + <handler event="focus" phase="capturing"><![CDATA[ > + this.attachController(); > + let userContextId = gBrowser.selectedTab.linkedBrowser.getAttribute("usercontextid") || 0; > + this.mController.userContextId = userContextId; Nit: use gBrowser.selectedBrowser.contentPrincipal.originAttributes.userContextId directly, which is also guaranteed to exist and be an int (rather than the attribute, which is a string).

(In reply to :Gijs Kruitbosch from comment #34) > numbers and could be simplified by using groupings in the regexp. I would > instead do: > > let userContextId = searchParam.match(/(?:^| )userContextId:(\d+)); Eh, that's missing the trailing '/' for the regexp, but you get the idea.

Comment on attachment 8758330 [details] [diff] [review] WIP Part 3: pass userContextId to autocomplete/search suggestion Review of attachment 8758330 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/SearchSuggestionController.jsm @@ +240,5 @@ > let method = (submission.postData ? "POST" : "GET"); > this._request.open(method, submission.uri.spec, true); > + > + if (userContextId != Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID) { > + this._request.setOriginAttributes({userContextId}); Thanks for the reminding of overriding origin attributes, I'll update my part 2 patch accordingly. And I removed the setPrivate() call because there should not have loadContext in PrivateBrowsing channel, otherwise it will assert in https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/netwerk/base/PrivateBrowsingChannel.h#38 or https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/netwerk/base/PrivateBrowsingChannel.h#93 depends on whether setPrivate or setOriginAttributes is called first. I forward to question to jdm, Hi Josh I tried to follow Jonas' suggestion Comment 20 to set origin attributes on the LoadContext and Loadinfo of the XHR. But the original code here will call setPrivate, which will trigger the assert I mentioned above, so my question is that can I set notificationCallbacks on the (private) channel? If I can, should I remove the setPrivate call here? Otherwise, Or should I modify my part 1 and Part 2 patches to create a LoadContext with specified origin attributes *and a usePrivateBrowsing flag* when I call XHR.setOriginAttributes? Thanks

Comment on attachment 8757838 [details] [diff] [review] Part 1: Use origin attributes as cstor arg for LoadContext Review of attachment 8757838 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/downloads/ApplicationReputation.cpp @@ +1008,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > // Set the Safebrowsing cookie jar, so that the regular Google cookie is not > // sent with this request. See bug 897516. > + DocShellOriginAttributes attrs(NECKO_SAFEBROWSING_APP_ID, false); Please do DocShellOriginAttributes attrs; attrs.mAppId = NECKO_SAFEBROWSING_APP_ID; We should deprecate the two-argument OA ctor. ::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp @@ +161,5 @@ > > // Create a custom LoadContext for SafeBrowsing, so we can use callbacks on > // the channel to query the appId which allows separation of safebrowsing > // cookies in a separate jar. > + DocShellOriginAttributes attrs(NECKO_SAFEBROWSING_APP_ID, false); Same here

Comment on attachment 8757839 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest Review of attachment 8757839 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.h @@ +102,5 @@ > mAppId = aAppId; > mInIsolatedMozBrowser = aInIsolatedMozBrowser; > } > + explicit DocShellOriginAttributes(const OriginAttributesDictionary& aOther) > + : OriginAttributes(aOther) {} This makes it possible to construct a DocShellOriginAttributes using a NeckoOriginAttributes without calling the Inherit* functions. @@ +123,5 @@ > mAppId = aAppId; > mInIsolatedMozBrowser = aInIsolatedMozBrowser; > } > + explicit NeckoOriginAttributes(const OriginAttributesDictionary& aOther) > + : OriginAttributes(aOther) {} Same here. ::: dom/base/nsXMLHttpRequest.cpp @@ +1672,5 @@ > } > > void > +nsXMLHttpRequest::SetOriginAttributes(const dom::OriginAttributesDictionary& aAttrs) > +{ You need to assert that mState is in a state where mChannel has already been created, but has not yet opened. @@ +1679,5 @@ > + DocShellOriginAttributes attrs(aAttrs); > + NeckoOriginAttributes neckoAttrs(aAttrs); > + > + nsCOMPtr<nsIInterfaceRequestor> loadContext = new LoadContext(attrs); > + nsresult rv = mChannel->SetNotificationCallbacks(loadContext); This will definitely break addons, and probably break internal code. The problem is that people expect that for XHR requests, that the notificationCallbacks object will be the XHR object itself. What you could do is to change nsXMLHttpRequest::GetInterface so that when someone tries to get the nsILoadContext, that they get the new object that you've instantiated. There's some chance that even that will break though. I'm not sure if there is code which expects to be able to QI from the nsILoadContext to the docshell, but I don't see a way that we could make that work. Whatever you do here you should get review from bz. I hate nsILoadContext :(

Hi BZ Please see Comment 11 for the background of this bug. Also Jonas' comment 20 and Comment 38 for his suggestions. I'd like to request some feedback from you since Jonas is uncertain about LoadContext here. Right now I have two questions in mind. 1. The search suggestions is using a private browsing channel, https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/toolkit/components/search/SearchSuggestionController.jsm#242 which shouldn't have LoadContext or LoadGroup from https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPrivateBrowsingChannel.idl#28, but Jonas' comment 11 says I should set LoadContext on the private channel, is Jonas still right here? Or should I do as my comment 36, to have a LoadContext with specified origin attributes and mInPrivateBrowsing is true, so I could remove the call setPrivate from SearchSuggestionController.jsm? 2. Currently in search suggestion, there's no notificationCallback in that channel (since it's private), If the answer to my first question is still 'we need to store notificationCallbacks in XHR', can you suggest how I should start without breaking add-ons? Thanks

> I'm not sure if there is code which expects to be able to QI from the nsILoadContext to the docshell, There could be, obviously, but there shouldn't be if people are not abusing loadcontext. Note also that we have a number of non-docshell nsILoadContext implementations, so that code would already be pretty broken if it operates on random channels. Anyway, for the specific questions from comment 39: 1. I have no idea what the private browsing channel bits are, sorry. I've never seen them before. I could try to figure them out, but it might be simpler to ask whoever added them. 2. Do what Jonas said: have GetInterface for nsILoadContext on XHR return the new thing.

Oh, and I have no opinion on whether we want to store notificationCallbacks on the channel here, but if that's what ends up getting used by the cookie code, and if it falls back to "send cookies", then presumably we do. Now maybe the fallback given lack of nsILoadContext should be to NOT send cookies, but this feels like it might break a bunch of stuff. :(

(In reply to Yoshi Huang[:allstars.chh] from comment #39) > 1. The search suggestions is using a private browsing channel, > https://dxr.mozilla.org/mozilla-central/rev/ > 4d63dde701b47b8661ab7990f197b6b60e543839/toolkit/components/search/ > SearchSuggestionController.jsm#242 > > which shouldn't have LoadContext or LoadGroup from > https://dxr.mozilla.org/mozilla-central/source/netwerk/base/ > nsIPrivateBrowsingChannel.idl#28, > > but Jonas' comment 11 says I should set LoadContext on the private channel, > is Jonas still right here? > Or should I do as my comment 36, to have a LoadContext with specified origin > attributes and mInPrivateBrowsing is true, so I could remove the call > setPrivate from SearchSuggestionController.jsm? When there's an nsIPrivateBrowsingChannel in use, that only means that the channel did not have any meaningful load context or load group available when the code was added. That should not prevent adding a load context or load group if doing so makes sense, but we should be careful to preserve the desired privacy status when doing so.

Comment on attachment 8758330 [details] [diff] [review] WIP Part 3: pass userContextId to autocomplete/search suggestion Review of attachment 8758330 [details] [diff] [review]: ----------------------------------------------------------------- Since bug 1269361 has just merged, you could set the private browsing status of the new origin attributes and avoid the problem entirely.

Hi Josh I've followd your suggestions to call setOriginAttributes({userContext:..., privateBrowsing:1} on the XHR, and removed the setPrivate call on the channel of the xhr. So far I met the assertions of mismatch of privateBrowsing between LoadContext and LoadInfo in NS_CompareLoadInfoAndLoadContext. So do we forget handling LoadInfo in bug 1269361 ? As LoadInfo also has origin attributes, but its usePrivateBrowsing flag comes from mSecurityFlags, not from origin attributes. Can you give me more suggestions with this? Thanks

rebase after Bug 1269361.

SetNotificationCallbacks on XHR itself.

Comment on attachment 8760206 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest v2 Review of attachment 8760206 [details] [diff] [review]: ----------------------------------------------------------------- Hi Jonas I've addressed your previous comment, however nsXMLHttpRequest already has a mNotificationCallbacks, and I am not pretty sure its purpose, so I add !mLoadContext to prevent GetInterface calls recursively, but not sure if there's a correct way here. Can you check this patch again? Thanks

This addressed Gijs' comments, and call setOriginAttributes({userContextId, privateBrowsingId:1}) base on jdm's suggestions.

Comment on attachment 8760208 [details] [diff] [review] WIP Part 3: pass userContextId to autocomplete/search suggestion v2 Review of attachment 8760208 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/SearchSuggestionController.jsm @@ +241,5 @@ > this._request.open(method, submission.uri.spec, true); > + > + if (userContextId != Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID) { > + this._request.setOriginAttributes({userContextId, > + privateBrowsingId : 1}); This ignores the value of privateMode, so I suspect it's the cause of the assertions you're seeing.

(In reply to Josh Matthews [:jdm] from comment #49) > > + > > + if (userContextId != Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID) { > > + this._request.setOriginAttributes({userContextId, > > + privateBrowsingId : 1}); > > This ignores the value of privateMode, so I suspect it's the cause of the > assertions you're seeing. Okay, I thought your comment 43 suggested I'd use privateBrowsingId, but if it doesn't work here I'll remove it and also remove calling setPrivate on the channel. Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #50) > (In reply to Josh Matthews [:jdm] from comment #49) > > > + > > > + if (userContextId != Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID) { > > > + this._request.setOriginAttributes({userContextId, > > > + privateBrowsingId : 1}); > > > > This ignores the value of privateMode, so I suspect it's the cause of the > > assertions you're seeing. > Okay, I thought your comment 43 suggested I'd use privateBrowsingId, but if > it doesn't work here I'll remove it and also remove calling setPrivate on > the channel. > > Thanks I meant something like let privateId = privateMode ? 1 : 0; this._request.setOriginAttributes({userContextId, privateBrowsingId: privateId}); Does that make sense?

See NS_QueryNotificationCallbacks [1] for how a channel's notification callbacks are generally used. Then see XMLHttpRequest::GetInterface for how the XHR code use mNotificationCallbacks to hook into that mechanism. So when someone wants a nsILoadContext off of a channel, they generally call NS_QueryNotificationCallbacks using a nsILoadContext iid. NS_QueryNotificationCallbacks will then call GetInterface on the notification callbacks using the nsILoadContext iid. In the XHR case, the XHR object is the notification callbacks. This means that we'll call XMLHttpRequest::GetInterface with the nsILoadContext iid. Since that iid is not one that XMLHttpRequest::GetInterface currently handles, XMLHttpRequest::GetInterface will call mNotificationCallbacks->GetInterface with the nsILoadContext iid. Currently, in most cases that will also not return anything. So we'll get back into NS_QueryNotificationCallbacks without a result from the GetInterface call. NS_QueryNotificationCallbacks will then get the notification callbacks off of the channel's loadgroup and then call GetInterface on that. Since the notification callbacks on the loadgroup is the docshell most of the time, that will get into nsDocShell::GetInterface with a nsILoadContext iid. nsDocShell::GetInterface will eventually (through nsDocLoader::GetInterface) end up QIing the docshell itself to nsILoadContext and so that's what will be returned to NS_QueryNotificationCallbacks and what will be returned to the code wanting an nsILoadContext. So to return a custom nsILoadContext, you should make XMLHttpRequest::GetInterface check for the nsILoadContext IID and return your custom nsILoadContext. But only when a custom originattribute has been set. [1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#634

do as Jonas suggested.

reorder args, also do as :jdm suggested, (Sorry, I misunderstood jdm's content before). There's still some failures on try, I am looking into them now.

fixed failures on try, I am working on tests now. Macro, can you give me some feedback about this? Thanks

Comment on attachment 8763072 [details] [diff] [review] Part 3: pass userContextId to autocomplete/search suggestion v4 Review of attachment 8763072 [details] [diff] [review]: ----------------------------------------------------------------- The approach looks ok, just a few things: ::: toolkit/components/autocomplete/nsIAutoCompleteController.idl @@ +24,5 @@ > > + /** > + * The userContextId of current selected tab. > + */ > + attribute unsigned short userContextId; It seems strange to attach the context id to the controller rather than to the input field since it's related to the tab since you are setting the value in the autocomplete binding, that is implementing nsIAutoCompleteInput, it looks simpler to expose a readonly contextId attribute from nsIAutoCompleteInput. You should still be able in the controller to access it from the input and set the searchParam if it's valid (indeed you should initialize it to 0 and check that, currently you are not initializing the value, that may thus be random). ::: toolkit/components/places/UnifiedComplete.js @@ +679,5 @@ > this._disablePrivateActions = params.has("disable-private-actions"); > this._inPrivateWindow = params.has("private-window"); > this._prohibitAutoFill = params.has("prohibit-autofill"); > > + let userContextId = searchParam.match(/(?:^| )userContextId:(\d+)/); could be worth to memoize the compiled regex, see the REGEXP_ constants at the top. @@ +680,5 @@ > this._inPrivateWindow = params.has("private-window"); > this._prohibitAutoFill = params.has("prohibit-autofill"); > > + let userContextId = searchParam.match(/(?:^| )userContextId:(\d+)/); > + this._userContextId = userContextId ? 1 * userContextId[1] : 0; parseInt?

add userContextId in nsIAutoCompleteInput.

Comment on attachment 8766686 [details] [diff] [review] Part 3: pass userContextId to autocomplete/search suggestion v5 Review of attachment 8766686 [details] [diff] [review]: ----------------------------------------------------------------- Hi Marco I've addressed your Comment 56, could you check this again? Thanks

added a xpcshell test

Comment on attachment 8766686 [details] [diff] [review] Part 3: pass userContextId to autocomplete/search suggestion v5 Review of attachment 8766686 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp @@ +1201,5 @@ > } > > + uint32_t userContextId; > + rv = input->GetUserContextId(&userContextId); > + NS_ENSURE_SUCCESS(rv, rv); I think we should not bail out if we can't get a context id, to support existing implementers. I'd rather change the next if as: if (NS_SUCCEEDED(GetUserContextId(&userContextId)) && userContextId != ... ) { ... } ::: toolkit/components/autocomplete/nsIAutoCompleteInput.idl @@ +153,5 @@ > */ > readonly attribute boolean noRollupOnCaretMove; > + > + /** > + * The userContextId of current selected tab. ..."of the current browser." I'd not specify tab, since it's not mandatory to have a "tab". ::: toolkit/components/places/UnifiedComplete.js @@ +654,5 @@ > * should exclude privacy-sensitive results as appropriate. > * * private-window: The search is taking place in a private window, > * possibly in permanent private-browsing mode. The search > * should exclude privacy-sensitive results as appropriate. > + * * userContextId: The userContextId of the selected tab. The other search params use a lowercase-with-dash convention, that I think would be good to maintain for coherence. Thus user-context-id? @@ +683,5 @@ > this._inPrivateWindow = params.has("private-window"); > this._prohibitAutoFill = params.has("prohibit-autofill"); > > + let userContextId = searchParam.match(REGEXP_USER_CONTEXT_ID); > + this._userContextId = userContextId ? parseInt(userContextId[1], 10) : 0; Should be Ci.nsIScriptSecurityManager.DEFAULT_USER_CONTEXT_ID rather than an hardcoded 0 ::: toolkit/components/satchel/nsFormFillController.cpp @@ +626,5 @@ > > +NS_IMETHODIMP > +nsFormFillController::GetUserContextId(uint32_t* aUserContextId) > +{ > + *aUserContextId = 0; nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID

Comment on attachment 8760205 [details] [diff] [review] Part 1: Use origin attributes as cstor arg for LoadContext v2 Review of attachment 8760205 [details] [diff] [review]: ----------------------------------------------------------------- Hi Jonas you already r+'ed this before, but I'd to send you another r? since Bug 1269361 is landed. The difference is only the mUserPrivateBrosing member in LoadContext.h Thanks ::: docshell/base/LoadContext.h @@ +98,4 @@ > : mTopFrameElement(nullptr) > , mNestedFrameId(0) > , mIsContent(false) > + , mUsePrivateBrowsing(aAttrs.mPrivateBrowsingId != 0) rebase since Bug 1269361 is landed.

rebase since XHR has been moved to dom/xhr/

Hi Marco I've addressed your previous comments, and also have some unit tests. (I've spent some time to figure out how to write a mochitest-chrome for this but in vain) Could you review this again? Thanks

Comment on attachment 8768731 [details] [diff] [review] Part 3: pass userContextId to autocomplete/search suggestion v6 Review of attachment 8768731 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/autocomplete/tests/unit/test_1244340.js @@ +11,5 @@ > +AutoCompleteSearch.prototype = Object.create(AutoCompleteSearchBase.prototype); > + > +function run_test() { > + run_next_test(); > +} run_test with a single call to run_next_test is no more needed. @@ +13,5 @@ > +function run_test() { > + run_next_test(); > +} > + > +add_test(function test_userContextId() { change to add_task(function* make doSearch return a promise that resolves to the searchparam, and here just let searchparam = yield doSearch(.... Assert.equal(searchParam, ... This will be more roubst against future asynchroni-zation of the searches. @@ +28,5 @@ > + aListener) { > + aOnCompleteCallback(aSearchParam); > + > + unregisterAutoCompleteSearch(search); > + run_next_test(); and remove this, just resolve the promise after unregistering the search. @@ +41,5 @@ > + > + controller.input = input; > + controller.startSearch(aString); > + > + } nit: too many newlines around this code. Please compact it a little bit. ::: toolkit/components/autocomplete/tests/unit/xpcshell.ini @@ +8,5 @@ > [test_393191.js] > [test_440866.js] > [test_463023.js] > [test_660156.js] > +[test_1244340.js] please name the test in a meaningful way. We stopped naming the tests with just the bug number some years ago.

addressed comment.

Comment on attachment 8760205 [details] [diff] [review] Part 1: Use origin attributes as cstor arg for LoadContext v2 This is already r+ by sicking in Comment 37

Comment on attachment 8768365 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest v6 Since Jonas is busy, I change the r? to :Bz as Jonas mentioned in Comment 38.

Comment on attachment 8768365 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest v6 The commit message here should describe what's actually being changed and what the point of the change is. That way I'd have at least a _chance_ of being able to tell whether the patch is what we want and whether it's correct. r- for this part. Is there a reason to do the InheritFromGenericAttributes thing instead of just having constructors that take either GenericOriginAttributes or OriginAttributesDictionary?

Comment on attachment 8768365 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest v6 Review of attachment 8768365 [details] [diff] [review]: ----------------------------------------------------------------- Hi BZ Sorry for not explaining first. I'll put some comments here first, next time I'll move them into commit message. Comment 11 explains the problem, the autocomplete is running under XUL, which uses the System Principal. But we'd like to autocomplete could respect the userContextId, so in different container tab, autocomplete won't leak any cookie that's from a different container tab. Jonas suggests me to add a function 'setOriginAttributes' in XHR in Comment 20, And Jonas Comment 38 and you Comment 40 have some comments regarding to the 'possibility of breaking addons' if someone will do QI from nsILoadContext to DocShell. I've run on try, the result looks okay, but I am not sure if I will break some addons..... So I'd like to request for your review on the XHR change. (In reply to Boris Zbarsky [:bz] from comment #69) > Is there a reason to do the InheritFromGenericAttributes thing instead of > just having constructors that take either GenericOriginAttributes or > OriginAttributesDictionary? See Jonas' comment 38, he thinks we should always use the Inherit* functions to convert the origin attributes, otherwise we might convert wrong origin attributes in the future. (For example, some attribute shouldn't be propagetd when we convert PrincipalOriginAttributes to NeckoOriginAttributes) More backgrounds of those Inherit* functions can be found in Bug 1209162. If you think I should split those changes regarding to BasePrincipal to a seperate part, and let bholley or sicking to review that, please let me know. Thanks

OK. So the commit message should talk about the fact that we're adding a way for chrome code to do an XHR that looks like it comes from a browsing context with some given set of originAttributes. That's reall the goal here, right? > I've run on try, the result looks okay, but I am not sure if I will break some addons..... Have you done any searches on the addons tree on dxr to see how or whether people use loadcontext?

Comment on attachment 8768365 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest v6 Review of attachment 8768365 [details] [diff] [review]: ----------------------------------------------------------------- The changes to mNotificationCallbacks seems wrong. Otherwise this seems like it's on the right path. ::: dom/xhr/XMLHttpRequestMainThread.cpp @@ +1587,5 @@ > + NeckoOriginAttributes neckoAttrs; > + neckoAttrs.InheritFromGenericAttributes(attrs); > + > + mLoadContext = new LoadContext(docShellAttrs); > + nsresult rv = mChannel->SetNotificationCallbacks(this); This means that when XHR::Send initializes mNotificationCallbacks, it'll get initialized to the wrong value. So this won't work. Most likely you can just remove this line, since XHR::Send will do the same thing. @@ +2784,5 @@ > // the channel and us. This cycle has to be manually broken if anything > // below fails. > + if (!mLoadContext) { > + mChannel->GetNotificationCallbacks(getter_AddRefs(mNotificationCallbacks)); > + mChannel->SetNotificationCallbacks(this); Why is this needed? @@ +2803,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { > // Drop our ref to the channel to avoid cycles. Also drop channel's > // ref to us to be extra safe. > + if (!mLoadContext) { > + mChannel->SetNotificationCallbacks(mNotificationCallbacks); Why is this needed? @@ +3395,5 @@ > } > > // Now give mNotificationCallbacks (if non-null) a chance to return the > // desired interface. > + if (mNotificationCallbacks && !mLoadContext) { Why is this needed?

remove if(!mLoadContext) Also add a xpcshell test for xhr.channel.notificationCallbacks.getInterface(nsILoadContext). The try is still running, also I'd like to ask how should I check addons code? My query is https://dxr.mozilla.org/addons/search?q=channel.notificationCallbacks.getInterface(Ci.nsILoadContext)&redirect=false However there are 89 results, should I check those one by one? Thanks

> However there are 89 results, should I check those one by one? That's what I would do, yes...

Comment on attachment 8770904 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest v7 Review of attachment 8770904 [details] [diff] [review]: ----------------------------------------------------------------- I assume that you've tested this and that it works? I.e. that you can get the correct cookie jar when using the awesomebar. And that no assertions fire in debug builds? If that's the case, then this looks good to me. But yeah, auditing addons sounds good, but I'll leave it to bz to determine which patterns in addons to look out for. Note that we don't need to worry about addons which simply use XHR and then call xhr.channel.notificationCallbacks.getInterface(Ci.nsILoadContext), since those addons won't call xhr.setOriginAttributes, and so won't be affected by this patch. What I'd be more worried about is addons somehow intercepting the XHR requests from chrome code. I'm not sure how an addon would do that though... ::: caps/BasePrincipal.cpp @@ +79,5 @@ > mPrivateBrowsingId = aAttrs.mPrivateBrowsingId; > } > > void > +DocShellOriginAttributes::InheritFromGenericAttributes(const GenericOriginAttributes& aAttrs) What we want here isn't really "inheriting". "iniheriting" happens when OriginAttribute information flows from docshell to documents to child documents to necko etc. In all of those cases we might need to modify which OA information is transferred. What we want here is more of a "set" operation. So I think simply adding a SetFromGeneric(const GenericOriginAttributes& aAttrs) function, which copies *all* OAs, should be added to one of the base classes.

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #75) > Comment on attachment 8770904 [details] [diff] [review] > Part 2: add setOriginAttributes in nsIXMLHttpRequest v7 > > Review of attachment 8770904 [details] [diff] [review]: > ----------------------------------------------------------------- > > I assume that you've tested this and that it works? I.e. that you can get > the correct cookie jar when using the awesomebar. And that no assertions > fire in debug builds? Yes. > ::: caps/BasePrincipal.cpp > @@ +79,5 @@ > > mPrivateBrowsingId = aAttrs.mPrivateBrowsingId; > > } > > > > void > > +DocShellOriginAttributes::InheritFromGenericAttributes(const GenericOriginAttributes& aAttrs) > > What we want here isn't really "inheriting". "iniheriting" happens when > OriginAttribute information flows from docshell to documents to child > documents to necko etc. > > In all of those cases we might need to modify which OA information is > transferred. > > What we want here is more of a "set" operation. So I think simply adding a > SetFromGeneric(const GenericOriginAttributes& aAttrs) function, which copies > *all* OAs, should be added to one of the base classes. The base class is OriginAttributes, and GenericOriginAttributes, with other PrincipalOriginAttributes, DocShellOriginAttributes, and NeckoOriginAttributes classes, they all inherit OriginAttributes. Also that in OriginAttributes, there's already a protected copy cstor https://dxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.h?q=caps/BasePrincipal.h&redirect_type=direct#61 Do you mean I should add a SetFromDict(const OriginAttributesDictionary& aAttrs) in OriginAttributes? Thanks

Hi Jonas Thanks for the feedback, please see my comment 77, for now I just rename those Inherit* to SetFromGenericOriginAttributes. Thanks

Comment on attachment 8771274 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest v8 Review of attachment 8771274 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that. But also check with bz what auditing he thinks should be done. ::: caps/BasePrincipal.cpp @@ +79,5 @@ > mPrivateBrowsingId = aAttrs.mPrivateBrowsingId; > } > > void > +DocShellOriginAttributes::SetFromGenericAttributes(const GenericOriginAttributes& aAttrs) I think the purpose of the SetFromGenericAttributes function is that it would set *all* the attributes. If I call xhr.setOriginAttributes({ addonId: 1, mSignedPkg: x }); I would expect to use those exact attributes when the network request goes out. So I think you can move this function to GenericOriginAttributes and have it set all attributes.

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #38) > There's some chance that even that will break though. I'm not sure if there > is code which expects to be able to QI from the nsILoadContext to the > docshell, but I don't see a way that we could make that work. > (In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #75) > but I'll leave it to bz to determine > which patterns in addons to look out for. Note that we don't need to worry > about addons which simply use XHR and then call > xhr.channel.notificationCallbacks.getInterface(Ci.nsILoadContext), since > those addons won't call xhr.setOriginAttributes, and so won't be affected by > this patch. > > What I'd be more worried about is addons somehow intercepting the XHR > requests from chrome code. I'm not sure how an addon would do that though... Hi Bz Do you have some suggestions for auditing the addons? Should I look for addons that will intercept XHR? for example : https://dxr.mozilla.org/addons/search?q=regexp:XMLHttpRequest.prototype.send\b&redirect=false but that still shows almost 500 results :(

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #78) > > void > > +DocShellOriginAttributes::SetFromGenericAttributes(const GenericOriginAttributes& aAttrs) > > I think the purpose of the SetFromGenericAttributes function is that it > would set *all* the attributes. If I call > > xhr.setOriginAttributes({ addonId: 1, mSignedPkg: x }); > > I would expect to use those exact attributes when the network request goes > out. > > So I think you can move this function to GenericOriginAttributes and have it > set all attributes. Hi Jonas Sorry I don't get this part. We still need NeckoOriginAttributes/DocShellOriginAttributes as they are needed by LoadInfo and LoadContext, respectively. What do you mean by 'move it to GenericOriginAttibutes' ? Thanks

> Do you have some suggestions for auditing the addons? You're just changing what the observed loadcontext is for some XHR channels without changing anything else, right? If so, examining addons that get the loadcontext seems like the way to go. You can, as Jonas says, probably assume that any addon that gets it from an XHR it controls is OK. The ones that get it from things like http on-modify-request or on-examine-response necko notifications are the ones that would be more worrying.

Hi BZ Thanks for your suggestions. My query is "getInterface(Ci.nsILoadContext) regexp:http-on-examine-response|http-on-modify-request" or https://dxr.mozilla.org/addons/search?q=getInterface(Ci.nsILoadContext)+regexp:http-on-examine-response|http-on-modify-request&redirect=false It shows only 4 results, and I think they all belong to the same addon. The code snippet is var i=e.subject.QueryInterface(Ci.nsIHttpChannel) ... var x=i.notificationCallbacks.getInterface(Ci.nsILoadContext); x.topFrameElement&&(g=x.topFrameElement._contentWindow||null)}catch(R){} It only checks the topFrameElement property of nsILoadContext, so it looks okay. Do you think my query is correct? Thanks

I have no idea; I don't know dxr's query language well enough to tell. But at the very least it would miss people using "Components.interfaces.nsILoadContext", if any. The way I would personally do this is grep for "nsILoadContext" and examine the resulting callsites.

(In reply to Boris Zbarsky [:bz] from comment #83) > The way I would personally do this is grep for "nsILoadContext" and examine > the resulting callsites. This. Even when fixing for other 'nsILoadContext' hits, the query from comment #82 is only matching things where the nsILoadContext and http-on-examine/modify stuff is on the same line, which is not a realistic limitation (ie there will be add-ons that use both but where they are not used on the same line).

(In reply to :Gijs Kruitbosch from comment #84) > This. Even when fixing for other 'nsILoadContext' hits, the query from > comment #82 is only matching things where the nsILoadContext and > http-on-examine/modify stuff is on the same line, which is not a realistic > limitation (ie there will be add-ons that use both but where they are not > used on the same line). Thanks, it indeed only search for patterns in the same line. Unfortunally DXR query cannot specify multi-lines, nor use ?= So I change the query to regexp:http-on-examine-response|http-on-modify-request|getInterface\(Ci.nsILoadContext\)|getInterface\(Components.interfaces.nsILoadContext\) https://dxr.mozilla.org/addons/search?q=regexp:http-on-examine-response|http-on-modify-request|getInterface\(Ci.nsILoadContext\)|getInterface\(Components.interfaces.nsILoadContext\)&redirect=false And check for addons that have both http-on- subject and getInterface(..nsILoadContext) manually. The result is huge (10k+), although most of them are just listening to http-on notifications. So far the result is most addons only get the topFrameElement or associatedWindow property of nsILoadContext. I haven't finished the checking yet, hopefully will finish this soon.

> The result is huge (10k+), although most of them are just listening to > http-on notifications. > So far the result is most addons only get the topFrameElement or > associatedWindow property of nsILoadContext. This patch changes what 'topFrameElement' and 'associatedWindow' return, right? I.e. xhr.channel.notificationCallbacks.getInterface(nsILoadContext).topFrameElement now returns null whereas it didn't used to? You should probably forward those properties, and probably other ones, to the existing nsILoadContext. I.e. you should enable passing an existing nsILoadContext to the new loadcontext ctor, and forward appropriate properties to it. > > So I think you can move this function to GenericOriginAttributes and have it > > set all attributes. > > Hi Jonas > Sorry I don't get this part. > We still need NeckoOriginAttributes/DocShellOriginAttributes as they are > needed by LoadInfo and LoadContext, respectively. > > What do you mean by 'move it to GenericOriginAttibutes' ? What I mean is that you should add a function OriginAttibutes::SetFromGeneric(const GenericOriginAttributes& aAttrs) to the OriginAttibutes class. This function copy all attributes. Then both NeckoOriginAttributes/DocShellOriginAttributes (and the other subclasses too) will inherit the function.

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #86) > > This patch changes what 'topFrameElement' and 'associatedWindow' return, > right? I.e. > > xhr.channel.notificationCallbacks.getInterface(nsILoadContext). > topFrameElement now returns null whereas it didn't used to? > In the original code (toolkit/components/search/SearchSuggestionController.jsm) After xhr.send, The notificationCallbacks will be XHR itself, and if we didn't call xhr.setOriginAttributes(), the mLoadContext in XMLHttpRequestMainThread will be null so actually getInterface(Ci.nsILoadContext) will throw. > You should probably forward those properties, and probably other ones, to > the existing nsILoadContext. I.e. you should enable passing an existing > nsILoadContext to the new loadcontext ctor, and forward appropriate > properties to it. > As explained above, do I still need to 'forward' the properties? As the nsILoadContext instance of notificationCallbacks will be only available if the addon has called xhr.setOriginAttributes,( which in turn will set mLoadContext of XMLHttpRequestMainThread.) > > > > So I think you can move this function to GenericOriginAttributes and have it > > > set all attributes. > > > > Hi Jonas > > Sorry I don't get this part. > > We still need NeckoOriginAttributes/DocShellOriginAttributes as they are > > needed by LoadInfo and LoadContext, respectively. > > > > What do you mean by 'move it to GenericOriginAttibutes' ? > > What I mean is that you should add a function > > OriginAttibutes::SetFromGeneric(const GenericOriginAttributes& aAttrs) > > to the OriginAttibutes class. This function copy all attributes. Then both > NeckoOriginAttributes/DocShellOriginAttributes (and the other subclasses > too) will inherit the function. Okay.

> so actually getInterface(Ci.nsILoadContext) will throw. Can you point me to the relevant code doing the getInterface call? I see nothing like that in SearchSuggestionController.jsm... But in general, people who fail to get an nsILoadContext off the channel notification callbacks would then retry with the loadgroup callbacks instead. So if we go from throwing to not throwing on the channel, we'd change behavior unless we forward to the loadgroup callbacks.

My concern is that an addon which does channel.notificationCallbacks.getInterface(nsILoadContext).topFrameElement To inspect requests coming from chrome will now get a different value than they did before. I'm not specifically speaking about what SearchSuggestionController.jsm does, but rather what addons do. I don't see why notificationCallbacks.getInterface(nsILoadContext) would throw. Could you explain?

(In reply to Boris Zbarsky [:bz] from comment #88) > > so actually getInterface(Ci.nsILoadContext) will throw. > > Can you point me to the relevant code doing the getInterface call? I see > nothing like that in SearchSuggestionController.jsm Sorry for the confusion, the code is in my local build, (that's what I originally thought that Jonas'd like to ask, but I think I misunderstood the question.) >... But in general, > people who fail to get an nsILoadContext off the channel notification > callbacks would then retry with the loadgroup callbacks instead. So if we > go from throwing to not throwing on the channel, we'd change behavior unless > we forward to the loadgroup callbacks. (In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #89) > My concern is that an addon which does > > channel.notificationCallbacks.getInterface(nsILoadContext).topFrameElement > > To inspect requests coming from chrome will now get a different value than > they did before. I'm not specifically speaking about what > SearchSuggestionController.jsm does, but rather what addons do. > > I don't see why notificationCallbacks.getInterface(nsILoadContext) would > throw. Could you explain? Sorry, I see your (bz's+sicking's) point now. Let me rephrase that if I understand correctly. My patch will call xhr.setOriginAttributes (in nsSearchSuggestions.jsm) And some addons will get the channel by listening to http-on-modify-request or http-on-examine-response. So let's say one addon happens to intercept the request from autocomplete, which is the XHR by nsSuggestSuggestions. Originally (before my patch), their channel.notificationCallbacks.getInterface(nsILoadContext) will throw (Because the notification is the XHR itself) , and they will go to the channel.loadGroup.notificationCallbacks to call getInterface(nsILoadContext), then grep some topFrameElement or assocaitedWindow property. But after my patch, they can get an instance of nsILoadContext from the channel.notificationCallbacks, however the nsILoadContext is the mLoadContext I created in my patch, and those topFrameElement or assocaitedWindow properties are not correct, which will have problems. So I'll have another part patch for this, to forward those property access of nsILoadContext to that one in loadGroup. Thanks

I've noticed some problems when I do the 'property forwarding' for LoadContext LoadContext doesn't implement some properties of nsILoadContext, for example https://dxr.mozilla.org/mozilla-central/source/docshell/base/LoadContext.cpp#76 I guess that's problem Jonas mentioned in Comment 38, some addons will expect to get the docshell, for example, https://dxr.mozilla.org/addons/source/628110/lib/contentPolicy.js#242 This addon expects the nsILoadContext will be the docshell, however if the loadContext here is the one I created, (i.e., the addon intercepts the XHR from chrome) the call to get associatedWindow will throw https://dxr.mozilla.org/addons/source/628110/lib/contentPolicy.js#245 So the addon should still be okay (since they want a docshell, not a loadContext) Am I correct? That means I don't have to do the property forwarding, right? Thanks

To be clear, the goal here is to not change behavior for addons that intercept http-on-modify-request and similar notifications. So if currently channel.notificationCallbacks.getInterface(...) throws in chrome JS (because XHR doesn't have a scriptable getInterface function), then in theory the goal would be to have it throw now too. However it seems to me that trying to perfectly keep the old behavior in all situations, while also inserting a new nsILoadContext, is going to get really messy. I think we have two options here: * Don't worry about addons compat. Instead do what's simple while being reasonably sure that we're at least not breaking internal code, and that we continue to implement interfaces as they should be. This likely would involve forwarding a number of properties on nsILoadContext to the docshell. * Stop using nsILoadContext to determine OriginAttributes, and thus which cookies, are used for network requests. Doing this will likely involve fixing bug 1264230 and bug 1264231, and then changing the necko code to get the OriginAttributes from nsILoadInfo rather than nsILoadContext. Once we've done that we can simply not worry about nsILoadContext in this bug, and instead just modify the OriginAttributes in the nsILoadInfo. I personally think the latter is the way to go. It is something we need to do anyway.

Hi Jonas and BZ Okay, I'll start to look into fixing nsILoadInfo. (In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #92) > * Don't worry about addons compat. Instead do what's simple while being > reasonably sure that we're at > least not breaking internal code, and that we continue to implement > interfaces as they should be. This > likely would involve forwarding a number of properties on nsILoadContext > to the docshell. > Can you explain more about forwarding properties here? To my understanding, the mNotificationCallbacks is the XHR itself(even without my patch), which is from the parent side (or chrome), but the addons wants the mNotificationCallbacks (which is also a instance of docshell) that come from content side, doesn't it make sense to let the code throw when getting those properties of nsILoadContext? I agree that we change the getInterface behavior here, but in the end the addon code will still throw.(even without my patch their code are already throwing exceptions if they try to intercept XHR from chrome) So I am not sure what do you mean by 'forwarding' here. Thanks

> the mNotificationCallbacks is the XHR itself(even without my patch), which is > from the parent side (or chrome), but the addons wants the mNotificationCallbacks > (which is also a instance of docshell) that come from content side, The XHR object and the docshell the load is happening in (if any) come from the same process.

Rather than dwell too much on the loadContext forwarding, I think we should go the nsILoadInfo route. I think that's the way to go.

use loadInfo instead.

Hi Jonas This is the interdiff after using loadInfo. If you have time, could you review this ? Thanks

Comment on attachment 8777291 [details] [diff] [review] Part 2: add setOriginAttributes in nsIXMLHttpRequest v9 Review of attachment 8777291 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below fixed. ::: caps/BasePrincipal.cpp @@ +303,5 @@ > +{ > + mAppId = aAttrs.mAppId; > + mInIsolatedMozBrowser = aAttrs.mInIsolatedMozBrowser; > + > + // addonId is computed from the principal URI and never propagated Since this is not a propagation function, but rather a setting function, we should set *all* members.

Pushed by yhuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a18d3c8af3c Part 1: Use origin attributes as cstor arg for LoadContext r=sicking https://hg.mozilla.org/integration/mozilla-inbound/rev/8627e28f153d Part 2: add setOriginAttributes in nsIXMLHttpRequest. r=sicking https://hg.mozilla.org/integration/mozilla-inbound/rev/bc99aaf0d06f Part 3: pass userContextId to search suggestions r=mak

https://hg.mozilla.org/mozilla-central/rev/4a18d3c8af3c https://hg.mozilla.org/mozilla-central/rev/8627e28f153d https://hg.mozilla.org/mozilla-central/rev/bc99aaf0d06f