SearchBouncer::IsNewTabPage might return incorrect result. |
||||||
Issue descriptionQuoting from issue 844428 : We can send the wrong NTP url to the renderer (SearchBouncer). This happens because InstantService listens for content::NOTIFICATION_RENDERER_PROCESS_CREATED notification but it does not handle multiple profiles correctly. While sending the NTP url to the renderer, it should check that its Profile matches with the RenderProcessHost's profile. This can possibly lead to other bugs since SearchBouncer::IsNewTabPage can return the wrong result. The NTPInterceptionTest.ContentScript test added in https://chromium-review.googlesource.com/c/chromium/src/+/1068607 should help repro this.
,
May 30 2018
Thanks for filing! Over to the new NTP owners. This should be an easy fix. (Famous last words)
,
Jun 5 2018
,
Sep 16
,
Sep 17
Could you tell me how to reproduce this? I'm a little bit confused by the description.
,
Sep 17
There's not much to repro. The code at https://cs.chromium.org/chromium/src/chrome/browser/search/instant_service.cc?type=cs&sq=package:chromium&g=0&l=424 does not handle multiple profiles correctly. It sends the NTP url to the new renderer without checking if it belongs to its profile.
,
Sep 17
Is that mean if I have profile 1 & 2 opened, I change profile 1's new tab page url, and the profile 2's new tab page url will be changed? I open two profiles at the same time and both navigate to google.com. I change the search engine on profile 1 (will change the new tab page url). And switch to profile 2 and open a new tab page. But both profiles have their new tab page url set correctly. This line should ensure the new tab page url is decided by individual profile. https://cs.chromium.org/chromium/src/chrome/browser/search/instant_service.cc?rcl=ab8d4d5af3d20f2b07ef76b786b7d1d0f67d467c&l=353. Am I misunderstood anything?
,
Sep 17
The new tab page url set in the renderer might be incorrect. Hence SearchBouncer::IsNewTabPage may return the wrong result. I am not sure if it leads to any observable effect, but it can certainly lead to bugs. >> This line should ensure the new tab page url is decided by individual profile. https://cs.chromium.org/chromium/src/chrome/browser/search/instant_service.cc?rcl=ab8d4d5af3d20f2b07ef76b786b7d1d0f67d467c&l=353. But we do this for each created renderer without checking which profile the renderer belongs to.
,
Sep 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ed8084c1830a31555191eae75988ec0ffbf0e218 commit ed8084c1830a31555191eae75988ec0ffbf0e218 Author: Weilun Shi <sweilun@chromium.org> Date: Tue Sep 18 19:20:47 2018 [NTP] Make sure the renderer belong to that profile When there are multiply profiles opened at the same time. The renderer creation process will be called multiple times which might cause incorrect behavior. Make sure the renderer created only for the corresponding profile. Bug: 847606 Change-Id: I4d47d22220cab2197ad59feedacd3e6a5bc0dabb Reviewed-on: https://chromium-review.googlesource.com/1229193 Reviewed-by: Kristi Park <kristipark@chromium.org> Commit-Queue: Weilun Shi <sweilun@chromium.org> Cr-Commit-Position: refs/heads/master@{#592140} [modify] https://crrev.com/ed8084c1830a31555191eae75988ec0ffbf0e218/chrome/browser/search/instant_service.cc
,
Sep 18
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by karandeepb@chromium.org
, May 29 2018Labels: -Pri-3 Pri-2
Owner: treib@chromium.org
Status: Assigned (was: Untriaged)