Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949213002:
Don't update subframes on parent frame commits. (Closed)
|
Created:
4 years, 7 months ago by Avi (use Gerrit) Modified:
4 years, 7 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't update subframes on parent frame commits.
This reverts r371031.
This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there.
BUG=598043, 542299, 600534
, https://github.com/whatwg/html/issues/1191
TEST=disabling
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/b48cb31b164c516c109e751f1bd8d9f6eb24b147
Cr-Commit-Position: refs/heads/master@{#391906}
Patch Set 1 #Patch Set 2 : second fix #Patch Set 3 : #
Total comments: 15
Patch Set 4 : fix #
Messages
Total messages: 30 (15 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Description was changed from ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug masked a much deeper and severe problem. Discussion is ongoing there. BUG=598043,542299,https://github.com/whatwg/html/issues/1191 TEST=disabling ========== to ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug masked a much deeper and severe problem. Discussion is ongoing there. BUG=598043,542299,https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949213002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949213002/1
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949213002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949213002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
Description was changed from ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug masked a much deeper and severe problem. Discussion is ongoing there. BUG=598043,542299,https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043,542299,https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949213002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949213002/40001
avi@chromium.org changed reviewers: + creis@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM with nits, while we figure out a way forward. Please add 600534 to the CL description, since we can "fix" that one with this CL as well. (The reporter of that bug will likely be interested in this.) Also, looks like the BUG line got wrapped strangely, with a broken link to the spec bug. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1881: // This is a bug; we should not return here. Rather, we should continue on nit: Blank line before, and start with TODO(avi, creis). (It's not necessarily something you personally have to fix, but you're knowledgable about it.) https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1888: // example, see <https://crbug.com/598043> and the spec discussion at Let's also mention 600534. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3505: // See http://crbug.com/542299 and https://github.com/whatwg/html/issues/546 . // TODO(avi, creis): Find another fix for the NC_IN_PAGE_NAVIGATION crash. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3566: // to be gone had to be reintroduced. Can you clarify that the code below repros a NC_IN_PAGE_NAVIGATION kill (assuming that it does)? https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3570: return; nit: No need for return or the blank line below, given the #if 0. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3663: Same nits.
Description was changed from ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043,542299,https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043,542299,600534,https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043,542299,600534,https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043, 542299, 600534, https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949213002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949213002/60001
https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1881: // This is a bug; we should not return here. Rather, we should continue on On 2016/05/05 17:45:42, Charlie Reis wrote: > nit: Blank line before, and start with TODO(avi, creis). (It's not necessarily > something you personally have to fix, but you're knowledgable about it.) Done. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1888: // example, see <https://crbug.com/598043> and the spec discussion at On 2016/05/05 17:45:42, Charlie Reis wrote: > Let's also mention 600534. That's a Google-only bug; do we want to mention it? https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3505: // See http://crbug.com/542299 and https://github.com/whatwg/html/issues/546 . On 2016/05/05 17:45:42, Charlie Reis wrote: > // TODO(avi, creis): Find another fix for the NC_IN_PAGE_NAVIGATION crash. Done. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3566: // to be gone had to be reintroduced. On 2016/05/05 17:45:42, Charlie Reis wrote: > Can you clarify that the code below repros a NC_IN_PAGE_NAVIGATION kill > (assuming that it does)? It does not. This browsertest tested the basic functionality of getting the about:blank frame reset, which, if done, would prevent the kill. It doesn't actually go as far as testing the kill, which would be more complicated. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3570: return; On 2016/05/05 17:45:42, Charlie Reis wrote: > nit: No need for return or the blank line below, given the #if 0. Done. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3663: On 2016/05/05 17:45:42, Charlie Reis wrote: > Same nits. Done.
Thanks. Still LGTM with a request below. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1888: // example, see <https://crbug.com/598043> and the spec discussion at On 2016/05/05 19:06:21, Avi wrote: > On 2016/05/05 17:45:42, Charlie Reis wrote: > > Let's also mention 600534. > > That's a Google-only bug; do we want to mention it? Ah. No, just listing it in the CL description should be sufficient then. https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3566: // to be gone had to be reintroduced. On 2016/05/05 19:06:21, Avi wrote: > On 2016/05/05 17:45:42, Charlie Reis wrote: > > Can you clarify that the code below repros a NC_IN_PAGE_NAVIGATION kill > > (assuming that it does)? > > It does not. > > This browsertest tested the basic functionality of getting the about:blank frame > reset, which, if done, would prevent the kill. It doesn't actually go as far as > testing the kill, which would be more complicated. Oh! I must have misunderstood when reviewing the original CL. Presumably we'll reopen 542299 as a result of landing this, so can you mention there what's needed to test the actual renderer kill?
https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1949213002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3566: // to be gone had to be reintroduced. On 2016/05/05 19:34:22, Charlie Reis wrote: > Presumably we'll reopen 542299 as a result of landing this Yep. > so can you mention > there what's needed to test the actual renderer kill? The renderer kill is when the iframe does an in-page navigation; if it navigates from about:blank we're fine, but if the iframe fails to reset to about:blank then it in-page navigates from the old URL and dies. I didn't look to see how they make about:blank navigate in-frame.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949213002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949213002/60001
Message was sent while issue was closed.
Description was changed from ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043, 542299, 600534, https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043, 542299, 600534, https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043, 542299, 600534, https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Don't update subframes on parent frame commits. This reverts r371031. This reverts a genuine bugfix, but the bug is needed to mask a much deeper and severe problem. Discussion is ongoing there. BUG=598043, 542299, 600534, https://github.com/whatwg/html/issues/1191 TEST=disabling CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/b48cb31b164c516c109e751f1bd8d9f6eb24b147 Cr-Commit-Position: refs/heads/master@{#391906} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b48cb31b164c516c109e751f1bd8d9f6eb24b147 Cr-Commit-Position: refs/heads/master@{#391906} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
