Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
317093002:
Show confirmation dialog for unsecure signin (Closed)
|
Created:
6 years, 6 months ago by guohui Modified:
6 years, 4 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionShow confirmation dialog for unsecure signin
Test under review in a separate CL https://codereview.chromium.org/418043002/.
BUG=368905
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289791
Patch Set 1 : #
Total comments: 3
Patch Set 2 : filter http request from native code instead of JS #
Total comments: 3
Patch Set 3 : renamed method #Patch Set 4 : rebased #
Total comments: 15
Patch Set 5 : charlie's comments addressed #Patch Set 6 : #Patch Set 7 : rebased #Messages
Total messages: 43 (0 generated)
Hey, could you please take a look at the CL? Thanks, Hui
On 2014/06/04 22:54:11, guohui wrote: > Hey, > > could you please take a look at the CL? > > Thanks, > > Hui Test coming soon ...
+xiyuan
LGTM But please also wait for Roger's review.
lgtm https://codereview.chromium.org/317093002/diff/20001/chrome/browser/resources... File chrome/browser/resources/gaia_auth/background.js (right): https://codereview.chromium.org/317093002/diff/20001/chrome/browser/resources... chrome/browser/resources/gaia_auth/background.js:222: // TOOD(guohui): Show password confirmation UI. Is this todo done now? Remove/fix comment?
https://codereview.chromium.org/317093002/diff/20001/chrome/browser/resources... File chrome/browser/resources/gaia_auth/background.js (right): https://codereview.chromium.org/317093002/diff/20001/chrome/browser/resources... chrome/browser/resources/gaia_auth/background.js:222: // TOOD(guohui): Show password confirmation UI. On 2014/06/08 03:01:56, Roger Tawa wrote: > Is this todo done now? Remove/fix comment? nope, this is not done yet, for more details please see crbug/348002.
since this CL needs to be merged into M36, i put the refactoring and tests in a separate CL 328773002.
The CQ bit was checked by guohui@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/317093002/20001
I didn't review this earlier since I was waiting for a test case to be added. Please see my comments as I'm worried this is not the best possible solution. https://codereview.chromium.org/317093002/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right): https://codereview.chromium.org/317093002/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:254: dict->GetBoolean("confirmUntrustedSignin", &confirm_untrusted_signin); Does this value come from the renderer? If yes, then a compromised renderer can lie. Considering we are trying to guard against HTTP, plaintext document can trivially deliver an exploit document and completely bypass this protection.
The CQ bit was unchecked by guohui@chromium.org
On 2014/06/10 17:11:34, guohui wrote: > The CQ bit was unchecked by mailto:guohui@chromium.org Thanks nasko, i think you are right, and i guess the same issue applies to CrOS as well. I have two ideas, 1.move the detection for untrusted requests from JS to native code in browser process. Any suggestion on where we should make the change? WebRequestAPI is hooked into chrome_network_delegate.cc, shall we do something similar? 2.background.js runs in the gaia auth extension process, not in the renderer process, so i guess if it could directly post the bit confirmUntrustedSignin to the browser process, then it could also work. Any thoughts?
On 2014/06/10 18:52:01, guohui wrote: > On 2014/06/10 17:11:34, guohui wrote: > > The CQ bit was unchecked by mailto:guohui@chromium.org > > Thanks nasko, i think you are right, and i guess the same issue applies to CrOS > as well. I have two ideas, > > 1.move the detection for untrusted requests from JS to native code in browser > process. Any suggestion on where we should make the change? WebRequestAPI is > hooked into chrome_network_delegate.cc, shall we do something similar? I think this road will involve mode complex solution, so we should try to avoid it. > 2.background.js runs in the gaia auth extension process, not in the renderer > process, so i guess if it could directly post the bit confirmUntrustedSignin to > the browser process, then it could also work. This is likely the better approach. Adding native code is going to be more error prone and it should be more straightforward to just send the bit of data from the extension process to the browser. The extension process shouldn't be under direct control of the attacker, so it is much better.
On 2014/06/10 19:51:58, nasko wrote: > On 2014/06/10 18:52:01, guohui wrote: > > On 2014/06/10 17:11:34, guohui wrote: > > > The CQ bit was unchecked by mailto:guohui@chromium.org > > > > Thanks nasko, i think you are right, and i guess the same issue applies to > CrOS > > as well. I have two ideas, > > > > 1.move the detection for untrusted requests from JS to native code in browser > > process. Any suggestion on where we should make the change? WebRequestAPI is > > hooked into chrome_network_delegate.cc, shall we do something similar? > > I think this road will involve mode complex solution, so we should try to avoid > it. > > > 2.background.js runs in the gaia auth extension process, not in the renderer > > process, so i guess if it could directly post the bit confirmUntrustedSignin > to > > the browser process, then it could also work. > > This is likely the better approach. Adding native code is going to be more error > prone and it should be more straightforward to just send the bit of data from > the extension process to the browser. The extension process shouldn't be under > direct control of the attacker, so it is much better. any suggestion how to do it? thought we could use chrome.send, but seems it is only available in webUI process.
On 2014/06/10 20:04:50, guohui wrote: > On 2014/06/10 19:51:58, nasko wrote: > > On 2014/06/10 18:52:01, guohui wrote: > > > On 2014/06/10 17:11:34, guohui wrote: > > > > The CQ bit was unchecked by mailto:guohui@chromium.org > > > > > > Thanks nasko, i think you are right, and i guess the same issue applies to > > CrOS > > > as well. I have two ideas, > > > > > > 1.move the detection for untrusted requests from JS to native code in > browser > > > process. Any suggestion on where we should make the change? WebRequestAPI is > > > hooked into chrome_network_delegate.cc, shall we do something similar? > > > > I think this road will involve mode complex solution, so we should try to > avoid > > it. > > > > > 2.background.js runs in the gaia auth extension process, not in the renderer > > > process, so i guess if it could directly post the bit confirmUntrustedSignin > > to > > > the browser process, then it could also work. > > > > This is likely the better approach. Adding native code is going to be more > error > > prone and it should be more straightforward to just send the bit of data from > > the extension process to the browser. The extension process shouldn't be under > > direct control of the attacker, so it is much better. > > any suggestion how to do it? thought we could use chrome.send, but seems it is > only available in webUI process. Can you tell me which pieces of JavaScript run where? Ultimately, the call that gets the bit is CompleteLogin on the browser side. Looking at the patch above, it seems that it originates from gaia_auth/main.js, but I'm not certain.
main.js got the bit from a message sent from background.js at line 230. I think background.js runs in the gaia auth extension process thus it should be safe?
ping? anyone has any idea?
On 2014/06/11 15:35:30, guohui wrote: > ping? anyone has any idea? chatted with kalman@, it seems there is no general way of sending a message from an extension process to C++ webUI handler, we need to make our own private API for it, not sure if it justifies the effort.
On 2014/06/11 17:57:23, guohui wrote: > On 2014/06/11 15:35:30, guohui wrote: > > ping? anyone has any idea? > > chatted with kalman@, it seems there is no general way of sending a message from > an extension process to C++ webUI handler, we need to make our own private API > for it, not sure if it justifies the effort. This means that we should track in the browser process whether the signin process navigates through HTTP then.
On 2014/06/11 18:07:30, nasko wrote: > On 2014/06/11 17:57:23, guohui wrote: > > On 2014/06/11 15:35:30, guohui wrote: > > > ping? anyone has any idea? > > > > chatted with kalman@, it seems there is no general way of sending a message > from > > an extension process to C++ webUI handler, we need to make our own private API > > for it, not sure if it justifies the effort. > > This means that we should track in the browser process whether the signin > process navigates through HTTP then. so does my original proposal of hooking into chrome_network_delegate.cc makes sense? or any other better idea?
On 2014/06/11 18:08:21, guohui wrote: > On 2014/06/11 18:07:30, nasko wrote: > > On 2014/06/11 17:57:23, guohui wrote: > > > On 2014/06/11 15:35:30, guohui wrote: > > > > ping? anyone has any idea? > > > > > > chatted with kalman@, it seems there is no general way of sending a message > > from > > > an extension process to C++ webUI handler, we need to make our own private > API > > > for it, not sure if it justifies the effort. > > > > This means that we should track in the browser process whether the signin > > process navigates through HTTP then. > > so does my original proposal of hooking into chrome_network_delegate.cc makes > sense? or any other better idea? I don't know exactly how your code is structured, but WebContentsObserver is probably the easier route, as it has notifications for all navigation events. It is also code that existed in the old style signin code, so you can probably borrow/reference it.
On 2014/06/11 18:10:08, nasko wrote: > On 2014/06/11 18:08:21, guohui wrote: > > On 2014/06/11 18:07:30, nasko wrote: > > > On 2014/06/11 17:57:23, guohui wrote: > > > > On 2014/06/11 15:35:30, guohui wrote: > > > > > ping? anyone has any idea? > > > > > > > > chatted with kalman@, it seems there is no general way of sending a > message > > > from > > > > an extension process to C++ webUI handler, we need to make our own private > > API > > > > for it, not sure if it justifies the effort. > > > > > > This means that we should track in the browser process whether the signin > > > process navigates through HTTP then. > > > > so does my original proposal of hooking into chrome_network_delegate.cc makes > > sense? or any other better idea? > > I don't know exactly how your code is structured, but WebContentsObserver is > probably the easier route, as it has notifications for all navigation events. It > is also code that existed in the old style signin code, so you can probably > borrow/reference it. hmm so you suggest to monitor DidNavigateAnyFrame event i guess? i ll try that.
Sorry for the long delay. Could you please take a look at the latest patch? I moved the filter logic from JS to native code. Thanks! https://codereview.chromium.org/317093002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/login/saml/saml_browsertest.cc (right): https://codereview.chromium.org/317093002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/login/saml/saml_browsertest.cc:389: GetLoginUI()->GetWebContents(), NULL, "signin-frame"); @xiyuan, do we have a known parent frame URL here? https://codereview.chromium.org/317093002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/317093002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:333: frame->ExecuteJavaScript(base::ASCIIToUTF16(code)); @xiyuan, do we have a known parent frame URL here?
btw, test is coming soon...
Add the same question for xiyuan@ to the patch 3. https://codereview.chromium.org/317093002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/317093002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:332: web_ui()->GetWebContents(), NULL, "signin-frame"); @xiyuan, do we have a known parent frame URL here?
please track the progress for test in https://codereview.chromium.org/328773002/. It requires non-trivial refactoring thus keep it separate. Will upload an updated patch in the test CL tmrw.
https://codereview.chromium.org/317093002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/317093002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:332: web_ui()->GetWebContents(), NULL, "signin-frame"); On 2014/07/22 22:07:58, guohui wrote: > @xiyuan, do we have a known parent frame URL here? It should be the same as kGaiaExtOrigin in InlineLoginHandlerImpl::DidCommitProvisionalLoadForFrame. The name "GetGaiaAuthIframe" is a bit confusing. It actually returns the iframe inside gaia_auth/main.html.
https://codereview.chromium.org/317093002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/317093002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:332: web_ui()->GetWebContents(), NULL, "signin-frame"); Changed the name to "GetAuthIframe", since it could be any iframe on the auth page as specified by the parent frame name and parent frame url.
On 2014/07/23 16:05:25, guohui wrote: > https://codereview.chromium.org/317093002/diff/120001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): > > https://codereview.chromium.org/317093002/diff/120001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:332: > web_ui()->GetWebContents(), NULL, "signin-frame"); > Changed the name to "GetAuthIframe", since it could be any iframe on the auth > page as specified by the parent frame name and parent frame url. ping?
SLGTM
On 2014/07/23 17:53:10, xiyuan wrote: > SLGTM Thanks xiyuan! @nasko, could you please take another look? Thanks!
Thanks. There's a lot of code here I'm not familiar enough with to review, but using a WebContentsObserver to watch for commits of untrusted URLs in the sign-in process sounds great to me. A few questions below. (Nasko, feel free to chime in if you see this as well.) https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right): https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:229: if (url.spec() != url::kAboutBlankURL && Let's put a comment here saying that loading any untrusted (e.g., HTTP) URLs in the privileged sign-in process should require confirmation before the sign in takes effect. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:231: !signin::IsContinueUrlForWebBasedSigninFlow(url)) { Does the continue URL get loaded in the sign-in process? Can it be HTTP? That could be a loophole. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:246: content::WebContentsObserver::Observe(contents); I think this means we only observe when the WebContents is used for sign-in? That's good. Is there a point at which we can stop observing (e.g., Observe(NULL))? https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_ui.cc (right): https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_ui.cc:90: else nit: No need for an else block, since we return in the if.
Thanks Charlie for your quick review! https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right): https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:229: if (url.spec() != url::kAboutBlankURL && On 2014/07/23 21:42:00, Charlie Reis wrote: > Let's put a comment here saying that loading any untrusted (e.g., HTTP) URLs in > the privileged sign-in process should require confirmation before the sign in > takes effect. Done. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:229: if (url.spec() != url::kAboutBlankURL && On 2014/07/23 21:42:00, Charlie Reis wrote: > Let's put a comment here saying that loading any untrusted (e.g., HTTP) URLs in > the privileged sign-in process should require confirmation before the sign in > takes effect. Done. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:231: !signin::IsContinueUrlForWebBasedSigninFlow(url)) { On 2014/07/23 21:42:00, Charlie Reis wrote: > Does the continue URL get loaded in the sign-in process? Can it be HTTP? That > could be a loophole. The continue URL does get loaded in the signin process at the end of signin flow. It is a fixed url as defined at, https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig... Basically it is a blank page created solely for the chrome signin process that is used to signal the completion of signin flow. And in my test cl, i actually change the continue url to a local page in the gaia auth extension. Either way this should not expose any security risk. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:246: content::WebContentsObserver::Observe(contents); On 2014/07/23 21:42:00, Charlie Reis wrote: > I think this means we only observe when the WebContents is used for sign-in? > That's good. > > Is there a point at which we can stop observing (e.g., Observe(NULL))? We don't explicitly stop observing, so the observation will be stopped upon destruction, which happens when the signin tab is closed. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_ui.cc (right): https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_ui.cc:90: else On 2014/07/23 21:42:00, Charlie Reis wrote: > nit: No need for an else block, since we return in the if. Done.
Thanks. Speaking of tests, this needs one where we verify that loading an HTTP URL into the sign-in process causes a confirmation prompt. You mentioned a separate test CL-- if that's for this change, can we move it into this CL? Better to land them together. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right): https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:231: !signin::IsContinueUrlForWebBasedSigninFlow(url)) { On 2014/07/23 21:57:51, guohui wrote: > On 2014/07/23 21:42:00, Charlie Reis wrote: > > Does the continue URL get loaded in the sign-in process? Can it be HTTP? > That > > could be a loophole. > > The continue URL does get loaded in the signin process at the end of signin > flow. It is a fixed url as defined at, > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig... > > Basically it is a blank page created solely for the chrome signin process that > is used to signal the completion of signin flow. > > And in my test cl, i actually change the continue url to a local page in the > gaia auth extension. Either way this should not expose any security risk. But it can be specified manually as a URL parameter, right? That means we don't have control over it, so I'm not sure why it's considered trusted. Perhaps I'm misunderstanding, but if it gets loaded in the process then it seems like it poses a risk. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:246: content::WebContentsObserver::Observe(contents); On 2014/07/23 21:57:51, guohui wrote: > On 2014/07/23 21:42:00, Charlie Reis wrote: > > I think this means we only observe when the WebContents is used for sign-in? > > That's good. > > > > Is there a point at which we can stop observing (e.g., Observe(NULL))? > > We don't explicitly stop observing, so the observation will be stopped upon > destruction, which happens when the signin tab is closed. Is it necessary to keep it around that long? It seems like unnecessary overhead once the sign in flow is done.
Yup the other test CL 328773002 verifies that loading http will trigger confirmation before signin completes. The test requires some non-trivial refactoring, and I assume we want to merge this CL to M37 (and even M36), thus I prefer to keep it separate to minimize the risk, what do you think? https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right): https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:231: !signin::IsContinueUrlForWebBasedSigninFlow(url)) { https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig... IsContinueUrlForWebBasedSigninFlow checks for a hardcoded URL thus we do have control over it. In some cases (webstore signin) we allow a customized continue URL, but the customized continue URL param does not override the hardcoded url in the method IsContinueUrlForWebBasedSigninFlow, thus the check should be good in all cases. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:246: content::WebContentsObserver::Observe(contents); sorry i made a mistake in my earlier comment, this class is destroyed when the inline signin is closed or when the tab navigates to some other URL. And once signin completes, chrome will immediately navigate to either ntp or settings page or close the tab, thus there should be not much overhead.
On 2014/07/23 23:19:47, guohui wrote: > Yup the other test CL 328773002 verifies that loading http will trigger > confirmation before signin completes. The test requires some non-trivial > refactoring, and I assume we want to merge this CL to M37 (and even M36), thus I > prefer to keep it separate to minimize the risk, what do you think? > I'd like to have the test in place when we land this. Here's one option: 1) Land the test CL first, with the test disabled. 2) Land this CL, including a line to enable the test. (The try jobs will thus verify that the test is passing as we land the fix.) 3) Once it bakes, merge this CL to earlier branches but omit the line that enables the test (since the other branches won't have the test). Ideally we'd merge the test as well, but I understand that merges get much harder when refactoring is required. Does this approach sound reasonable? https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right): https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:231: !signin::IsContinueUrlForWebBasedSigninFlow(url)) { On 2014/07/23 23:19:46, guohui wrote: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sig... > > IsContinueUrlForWebBasedSigninFlow checks for a hardcoded URL thus we do have > control over it. > > In some cases (webstore signin) we allow a customized continue URL, but the > customized continue URL param does not override the hardcoded url in the method > IsContinueUrlForWebBasedSigninFlow, thus the check should be good in all cases. Acknowledged. https://codereview.chromium.org/317093002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:246: content::WebContentsObserver::Observe(contents); On 2014/07/23 23:19:46, guohui wrote: > sorry i made a mistake in my earlier comment, this class is destroyed when the > inline signin is closed or when the tab navigates to some other URL. Ah, good. That sounds good to me. > And once > signin completes, chrome will immediately navigate to either ntp or settings > page or close the tab, thus there should be not much overhead.
I'd cite with Charlie that I'd like to see a test added that ensures there is coverage for browsing through http. Otherwise the code looks good.
thanks everyone! I sent the test CL for review at https://codereview.chromium.org/328773002/. Unfortunately the test CL has dependency on this one (moving and renaming GetGaiaAuthIframe), and I only plan to merge this one since the test CL adds much complexity. What about I hold this CL until the test CL is lgtm'ed by all the owners? Once the test CL is lgtm'ed, I will submit this one and then the test CL.
On 2014/07/24 16:51:29, guohui wrote: > thanks everyone! > > I sent the test CL for review at https://codereview.chromium.org/328773002/. > Unfortunately the test CL has dependency on this one (moving and renaming > GetGaiaAuthIframe), and I only plan to merge this one since the test CL adds > much complexity. > > What about I hold this CL until the test CL is lgtm'ed by all the owners? Once > the test CL is lgtm'ed, I will submit this one and then the test CL. Would it be possible to send a try job with both patches to make sure the test passes (before landing either)? Then it would be fine to land this one closely followed by the test CL.
Definitely, i verified the test locally that it failed without my patch and passed with my patch, i ll send it to trybot as well, thanks!
The CQ bit was checked by guohui@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/317093002/250001
Message was sent while issue was closed.
Committed patchset #7 (250001) as 289791 |
