Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1848813005:
Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). (Closed)
|
Created:
4 years, 8 months ago by Charlie Reis Modified:
4 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, Nate Chapin, clamy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix HistoryEntry corruption when commit isn't for provisional entry (try #2).
BUG=597322, 600238
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/82500315884357312b3caafb49d347ef011c296f
Cr-Commit-Position: refs/heads/master@{#386785}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #
Total comments: 10
Patch Set 3 : Fix nits #Patch Set 4 : Add second test, plus updated fix. #Patch Set 5 : Rebase #
Total comments: 4
Patch Set 6 : Fix test nits #
Messages
Total messages: 28 (12 generated)
Description was changed from ========== Add test for HistoryEntry corruption. BUG=597322 ========== to ========== Add test for HistoryEntry corruption. BUG=597322 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
creis@chromium.org changed reviewers: + alexmos@chromium.org
Alex, can you review this test? CC'ing Nate for FYI, since he's reviewing the fix in a separate CL (https://codereview.chromium.org/1848103004). (This test CL can't land until Camille's CL lands, and I want to get the fix in and merged if possible.) https://codereview.chromium.org/1848813005/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1848813005/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3579: TestNavigationManager subframe_delayer(shell()->web_contents(), frame_url_a2); This TestNavigationManager class is being introduced in https://codereview.chromium.org/1825523002/, which is having trouble landing due to the CQ. I've found it necessary for testing the bug effectively.
Great, LGTM with a couple of nits below. https://codereview.chromium.org/1848813005/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1848813005/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3579: TestNavigationManager subframe_delayer(shell()->web_contents(), frame_url_a2); On 2016/04/01 17:28:15, Charlie Reis (slow til 4-8) wrote: > This TestNavigationManager class is being introduced in > https://codereview.chromium.org/1825523002/, which is having trouble landing due > to the CQ. I've found it necessary for testing the bug effectively. Acknowledged. Looks like it landed now. It's great to be able to do these things in tests now. https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3534: NavigateToURL(shell(), url_a); nit: EXPECT_TRUE https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3535: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); Does this add anything over NavigateToURL? https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3544: ASSERT_NE(nullptr, frame); nit: this seems redundant with the child_count ASSERT, and also can child_at ever return nullptr (it seems it would have an out-of-bounds crash instead)? https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3551: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); This shouldn't be necessary anymore after Nasko fixed NavigateFrameToURL/TestFrameNavigationObserver to wait for DidStopLoading. https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3562: NavigateToURL(shell(), url_b); nit: EXPECT_TRUE
Thanks! I'll land this once I take a look at https://crbug.com/600238, in case I need to revert the fix. https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3534: NavigateToURL(shell(), url_a); On 2016/04/02 00:18:54, alexmos wrote: > nit: EXPECT_TRUE Done. https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3535: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); On 2016/04/02 00:18:54, alexmos wrote: > Does this add anything over NavigateToURL? Done. https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3544: ASSERT_NE(nullptr, frame); On 2016/04/02 00:18:54, alexmos wrote: > nit: this seems redundant with the child_count ASSERT, and also can child_at > ever return nullptr (it seems it would have an out-of-bounds crash instead)? Done. https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3551: EXPECT_TRUE(WaitForLoadStop(shell()->web_contents())); On 2016/04/02 00:18:54, alexmos wrote: > This shouldn't be necessary anymore after Nasko fixed > NavigateFrameToURL/TestFrameNavigationObserver to wait for DidStopLoading. Done. https://codereview.chromium.org/1848813005/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3562: NavigateToURL(shell(), url_b); On 2016/04/02 00:18:54, alexmos wrote: > nit: EXPECT_TRUE Done.
LGTM
Description was changed from ========== Add test for HistoryEntry corruption. BUG=597322 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). BUG=597322 TEST=See bug for repro steps. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
creis@chromium.org changed reviewers: + japhet@chromium.org
Ok, I've added an updated fix and a second test to this CL, rather than having it just be the test. Nate: Can you review the fix in history_controller.cc? The comment there should explain it. Alex: Can you review the ForwardRedirectWithNoCommittedEntry test? That repros the crash that caused the original fix (https://codereview.chromium.org/1848103004) to be reverted. Thanks!
Description was changed from ========== Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). BUG=597322 TEST=See bug for repro steps. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). BUG=597322, 600238 TEST=See bug for repro steps. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The new test LGTM. https://codereview.chromium.org/1848813005/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1848813005/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3641: EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); nit: no need for content:: https://codereview.chromium.org/1848813005/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3652: scoped_ptr<WebContentsImpl> new_tab( nit: have we switched to unique_ptr?
Thanks. New patch up. Sorry that a rebase crept into patch set 5 with the minor test fixes. (No changes to the fix.) https://codereview.chromium.org/1848813005/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/1848813005/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3641: EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script)); On 2016/04/11 17:14:05, alexmos wrote: > nit: no need for content:: Good point. It's silly that all the ExecuteScript calls have that in this file. I'll remove the rest of them next (to avoid making this CL harder to merge). https://codereview.chromium.org/1848813005/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:3652: scoped_ptr<WebContentsImpl> new_tab( On 2016/04/11 17:14:05, alexmos wrote: > nit: have we switched to unique_ptr? Yep, thanks for catching.
The CQ bit was checked by creis@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/1848813005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1848813005/100001
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by creis@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/1848813005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1848813005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1848813005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1848813005/100001
Message was sent while issue was closed.
Description was changed from ========== Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). BUG=597322, 600238 TEST=See bug for repro steps. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). BUG=597322, 600238 TEST=See bug for repro steps. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). BUG=597322, 600238 TEST=See bug for repro steps. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix HistoryEntry corruption when commit isn't for provisional entry (try #2). BUG=597322, 600238 TEST=See bug for repro steps. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/82500315884357312b3caafb49d347ef011c296f Cr-Commit-Position: refs/heads/master@{#386785} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/82500315884357312b3caafb49d347ef011c296f Cr-Commit-Position: refs/heads/master@{#386785} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
