New issue
Advanced search Search tips

Issue 802042 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

AddContentScriptToOneWebViewShouldNotInjectToTheOtherWebView in WebUIWebViewBrowserTest is flaky

Project Member Reported by mstensho@chromium.org, Jan 15 2018

Issue description

See https://ci.chromium.org/buildbot/chromium.memory/Linux%20CFI/5202

I also got this one to fail once while attempting to land something today.
 
Cc: dpa...@chromium.org
Labels: -Sheriff-Chromium
Owner: mcnee@chromium.org
Status: Assigned (was: Untriaged)
I can't get this to repro locally, though the Linux CFI bot may have different args.

The flakes failures appear to have started with: https://ci.chromium.org/buildbot/chromium.memory/Linux%20CFI/5162

I'm suspecting a change by mcnee@ which mentions changes to OOPIF timings. Those this may be incorrect, so feel free to pass this to the CCed WedUI owner dpapad@

Suspect change: https://chromium.googlesource.com/chromium/src/+/f79536bce8cea4de00a9fc57ca648362538b4b13

Failure from the logs:

JS call assumed failed, because JS console error(s) found.
../../chrome/browser/ui/webui/webui_webview_browsertest.cc:213: Failure
Value of: WebUIBrowserTest::RunJavascriptAsyncTest( "testAddContentScriptToOneWebViewShouldNotInjectToTheOtherWebView", base::Value(GetTestUrl("empty.html").spec()))
  Actual: false
Expected: true
Cc: jonr...@chromium.org
I'm landing a patch to disable this test in the interim: https://chromium-review.googlesource.com/c/chromium/src/+/866991


Project Member

Comment 3 by bugdroid1@chromium.org, Jan 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0937b24ec882796cbb53af75e70abca384099fd

commit b0937b24ec882796cbb53af75e70abca384099fd
Author: Jonathan <jonross@chromium.org>
Date: Mon Jan 15 16:08:48 2018

Disable flaking WebUI Test

WebUIWebViewBrowserTest.
AddContentScriptToOneWebViewShouldNotInjectToTheOtherWebView has become flaky,
I'm disabling it until it can be fixed.

TEST=WebUIWebViewBrowserTest.AddContentScriptToOneWebViewShouldNotInjectToTheOtherWebView
TBR=dpapad@chromium.org

Bug:  802042 
Change-Id: Icb58526e2cd742ce8db6ba58559558da7216d6e6
Reviewed-on: https://chromium-review.googlesource.com/866991
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529289}
[modify] https://crrev.com/b0937b24ec882796cbb53af75e70abca384099fd/chrome/browser/ui/webui/webui_webview_browsertest.cc

Comment 4 by mcnee@chromium.org, Jan 15 2018

Components: Platform>Apps>BrowserTag
Labels: -Pri-3 Pri-1
Reverting: https://chromium-review.googlesource.com/c/chromium/src/+/867210
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ac813c4bb11edc719106bb019172647c86d559c6

commit ac813c4bb11edc719106bb019172647c86d559c6
Author: Kevin McNee <mcnee@chromium.org>
Date: Mon Jan 15 17:32:54 2018

Revert "OOPIF guest view: Don't ACK AttachToEmbedderFrame before the attachment."

This reverts commit f79536bce8cea4de00a9fc57ca648362538b4b13.

Reason for revert: This caused WebUIWebViewBrowserTest.AddContentScriptToOneWebViewShouldNotInjectToTheOtherWebView to fail due to a null contentWindow.

Original change's description:
> OOPIF guest view: Don't ACK AttachToEmbedderFrame before the attachment.
> 
> In the OOPIF based guest version of |attachImpl$| we call
> |AttachIframeGuest| and provide a callback to be run after the
> attachment is done. However, the callback is run before the
> frame is actually swapped for a remote frame, as evidenced by
> the contentWindow still being local when the callback is run.
> 
> The relative order of |AttachToOuterWebContentsFrame| and
> |GuestViewMsg_AttachToEmbedderFrame_ACK| appears to have been
> accidentally changed in https://codereview.chromium.org/2472253002
> 
> We now send the ACK after the call to |AttachToOuterWebContentsFrame|
> so that the callback is run after the attachment.
> 
> Bug: None
> Change-Id: I5c33fab4ec2e8f6fd146b345e3a178db90912a31
> Reviewed-on: https://chromium-review.googlesource.com/862686
> Reviewed-by: James MacLean <wjmaclean@chromium.org>
> Commit-Queue: Kevin McNee <mcnee@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#528952}

TBR=wjmaclean@chromium.org,mcnee@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  802042 
Change-Id: I0e55483679bb75cf92e70e3b7c2e849e637378ef
Reviewed-on: https://chromium-review.googlesource.com/867210
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529307}
[modify] https://crrev.com/ac813c4bb11edc719106bb019172647c86d559c6/components/guest_view/browser/guest_view_message_filter.cc

Comment 6 by mcnee@chromium.org, Jan 15 2018

Status: Started (was: Assigned)
CL to reenable: https://chromium-review.googlesource.com/c/chromium/src/+/867610
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71916aa2b4a6dac828cfffb1ef0b2693093e4be7

commit 71916aa2b4a6dac828cfffb1ef0b2693093e4be7
Author: Kevin McNee <mcnee@chromium.org>
Date: Tue Jan 16 18:21:30 2018

Reenable AddContentScriptToOneWebViewShouldNotInjectToTheOtherWebView

The CL that caused this to start failing has been reverted.
https://chromium-review.googlesource.com/c/chromium/src/+/862686
https://chromium-review.googlesource.com/c/chromium/src/+/867210

Bug:  802042 
Change-Id: Ia80b1f9f3fa45b94963045e47272a52e6b747c3e
Reviewed-on: https://chromium-review.googlesource.com/867610
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529464}
[modify] https://crrev.com/71916aa2b4a6dac828cfffb1ef0b2693093e4be7/chrome/browser/ui/webui/webui_webview_browsertest.cc

Comment 8 by mcnee@chromium.org, Jan 16 2018

Status: Fixed (was: Started)

Comment 9 by mcnee@chromium.org, Jan 17 2018

I've relanded my CL with corrected logic:
https://chromium-review.googlesource.com/c/chromium/src/+/868277

I'll keep an eye on the flakiness dashboard for this test to make sure this new CL is okay.

Sign in to add a comment