New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 769502 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Out until 24 Jan
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 477150



Sign in to add a comment

After enabling PlzNavigate, fast/loader/recursive-before-unload-crash.html times out with --site-per-process

Project Member Reported by lukasza@chromium.org, Sep 27 2017

Issue description

REPRO STEPS:

$ DISPLAY=:20 third_party/WebKit/Tools/Scripts/run-webkit-tests -t gn --no-retry --additional-drt-flag=--site-per-process fast/loader/recursive-before-unload-crash.html


Note that

- The test is run with --site-per-process flag (passed via --additional-drt-flag=...)

- The test passes after adding --additional-drt-flag=--disable-browser-side-navigation
 

Comment 1 by nasko@chromium.org, Sep 27 2017

Cc: nasko@chromium.org
Owner: arthurso...@chromium.org
Reassigning to arthursonzogni@, as I won't have time to look into these failures soon.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 28 2017

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

commit d100bb101d90652a502b50edc67b4676c870cce4
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Sep 28 02:43:04 2017

Exclusions for PlzNavigate issues found on the Site Isolation Win bot.

NOTRY=true

Bug:  769502 
Bug:  769508 
Change-Id: Ib894feba443f5bbfbcd133f0ad73b9dd12e962ab
Tbr: alexmos@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/688223
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ɓukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504875}
[modify] https://crrev.com/d100bb101d90652a502b50edc67b4676c870cce4/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Status: Assigned (was: Untriaged)
I can repro. I am investigating....

Comment 4 Deleted

This layout test is equivalent to:
----------------------------------------
<script>
  testRunner.waitUntilDone();
  testRunner.dumpAsText();
  window.location.href="http://127.0.0.1:1234/";
  setTimeout(()=>testRunner.notifyDone(),0);
</script>
----------------------------------------

With OOPIF, the current RenderFrameImpl is swapped out when it is asked to navigate to "http://127.0.0.1:1234".

1) testRunner.notifyDone() causes ShellViewHostMsg_TestFinished to be sent, but it will be canceled here:
----------------------------------------
bool RenderWidget::Send(IPC::Message* message) {
  // Don't send any messages after the browser has told us to close, and filter
  // most outgoing messages while swapped out.
  if ((is_swapped_out_ &&
       !SwappedOutMessages::CanSendWhileSwappedOut(message)) ||
      closing_) {
    delete message;
    return false;
  }
  [...]
----------------------------------------

In parallel with 1), I think the second RenderFrame stops loading and then the BrowserMainRunner stops.

According to me, there is a race condition in the test. We can't say whether the navigation will happens before or after testRunner.notifyDone().

If OOPIF is enabled, we can execute testRunner.notifyDone() in the first process and the navigation in the second.
With PlzNavigate, the navigation goes in the browser process faster.
That may explain why it behave differently.

What do you think? I think we should update the test.
I wonder if this test is up to date. (created: 7 years ago, last-modification: 5 years ago).
Can we request a navigation in the OnBeforeUnload?
Blocking: 477150
Components: UI>Browser>Navigation
Labels: -Pri-3 Test-Layout Pri-2
Updating the test SGTM.  I don't know about requesting a navigation in OnBeforeUnload, but maybe other UI>Browser>Navigation folks can chime in.
Cc: -nasko@chromium.org
Owner: nasko@chromium.org
Nasko found that this test is obsolete and is removing it in https://chromium-review.googlesource.com/c/chromium/src/+/762520/, so reassigning to him to close this bug once that CL lands.

Comment 8 by nasko@chromium.org, May 4 2018

Status: Fixed (was: Assigned)
The test is now gone.

Sign in to add a comment