Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2140393002:
Don't clear pending NavigationParams until didStopLoading. (Closed)
|
Created:
4 years, 5 months ago by Charlie Reis Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, darin-cc_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't clear pending NavigationParams until didStopLoading.
Clearing them at the end of NavigateInternal causes problems when a
ScopedPageLoadDeferrer is in use.
Credit to thestig@ for the test framework changes.
BUG=626838
TEST=See bug for repro steps.
Committed: https://crrev.com/275d69fc4e8af8ab4a8e7d8fe621ca5b408c134d
Cr-Commit-Position: refs/heads/master@{#407239}
Patch Set 1 #Patch Set 2 : Update test #
Total comments: 2
Patch Set 3 : Fix wait bug. #
Total comments: 5
Patch Set 4 : Rebase #Patch Set 5 : Test fix. #
Messages
Total messages: 34 (18 generated)
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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't clear pending NavigationParams until didStopLoading. Clearing them at the end of NavigateInternal causes problems when a ScopedPageLoadDeferrer is in use. BUG=626838 TEST=See bug for repro steps. ========== to ========== Don't clear pending NavigationParams until didStopLoading. Clearing them at the end of NavigateInternal causes problems when a ScopedPageLoadDeferrer is in use. Credit to thestig@ for the test framework changes. BUG=626838 TEST=See bug for repro steps. ==========
creis@chromium.org changed reviewers: + avi@chromium.org, thestig@chromium.org
avi@: Can you review render_frame_impl.cc? Feel free to CQ while I'm out if you don't find issues, or else I can revisit on Wednesday. thestig@: Can you review the test? (Thanks for your help with it!)
https://codereview.chromium.org/2140393002/diff/20001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller_browsertest.cc (right): https://codereview.chromium.org/2140393002/diff/20001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_browsertest.cc:115: if (waiting_) Isn't this always true? It should go in RenderViewReady() below. Whoops. Copy + pasting is hard.
The CQ bit was unchecked by creis@chromium.org
https://codereview.chromium.org/2140393002/diff/20001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller_browsertest.cc (right): https://codereview.chromium.org/2140393002/diff/20001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_browsertest.cc:115: if (waiting_) On 2016/07/15 23:43:33, Lei Zhang wrote: > Isn't this always true? It should go in RenderViewReady() below. Whoops. Copy + > pasting is hard. Oops! Fixed.
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/v2/patch-status/codereview.chromium.or...
test lgtm, I'm pretty sure that was my fault.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
:/ https://codereview.chromium.org/2140393002/diff/40001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller_browsertest.cc (right): https://codereview.chromium.org/2140393002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_browsertest.cc:299: HistoryNavigateDuringPrint) { You're aware that this new test is red on the rel_ng trybots? https://codereview.chromium.org/2140393002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2140393002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:5644: pending_navigation_params_.reset(); OMG I hate pending params. When can we kill them entirely?
https://codereview.chromium.org/2140393002/diff/40001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller_browsertest.cc (right): https://codereview.chromium.org/2140393002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_browsertest.cc:299: HistoryNavigateDuringPrint) { On 2016/07/16 02:29:13, Avi wrote: > You're aware that this new test is red on the rel_ng trybots? Doh. I can help take a look. Maybe take over the CL if needed.
https://codereview.chromium.org/2140393002/diff/40001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller_browsertest.cc (right): https://codereview.chromium.org/2140393002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_browsertest.cc:299: HistoryNavigateDuringPrint) { On 2016/07/16 03:34:12, Lei Zhang wrote: > On 2016/07/16 02:29:13, Avi wrote: > > You're aware that this new test is red on the rel_ng trybots? > > Doh. I can help take a look. Maybe take over the CL if needed. Any ideas? It turns out ScopedPageLoadDeferrer is being used in some higher severity bugs, so we should get this landed soon. Thanks!
On 2016/07/20 21:32:26, Charlie Reis wrote: > Any ideas? It turns out ScopedPageLoadDeferrer is being used in some higher > severity bugs, so we should get this landed soon. > > Thanks! Ack. I did take a brief look and noticed on Windows the print preview dialog is not coming up, thus the wait for the dialog start hangs. Have not figured out why yet.
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/v2/patch-status/codereview.chromium.or...
thestig@: Thanks for finding how to fix the test! It's passing on the Win/Mac rel_ng bots now. avi@: Can you take another look? https://codereview.chromium.org/2140393002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2140393002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:5644: pending_navigation_params_.reset(); On 2016/07/16 02:29:13, Avi wrote: > OMG I hate pending params. When can we kill them entirely? Yes, I would love to see them gone. I'm guessing it's just a large plumbing job to get the necessary parts of them into Blink as NavigationState earlier, allowing them to follow the navigation.
Assuming things work, LGTM.
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 creis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2140393002/#ps80001 (title: "Test fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Don't clear pending NavigationParams until didStopLoading. Clearing them at the end of NavigateInternal causes problems when a ScopedPageLoadDeferrer is in use. Credit to thestig@ for the test framework changes. BUG=626838 TEST=See bug for repro steps. ========== to ========== Don't clear pending NavigationParams until didStopLoading. Clearing them at the end of NavigateInternal causes problems when a ScopedPageLoadDeferrer is in use. Credit to thestig@ for the test framework changes. BUG=626838 TEST=See bug for repro steps. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Don't clear pending NavigationParams until didStopLoading. Clearing them at the end of NavigateInternal causes problems when a ScopedPageLoadDeferrer is in use. Credit to thestig@ for the test framework changes. BUG=626838 TEST=See bug for repro steps. ========== to ========== Don't clear pending NavigationParams until didStopLoading. Clearing them at the end of NavigateInternal causes problems when a ScopedPageLoadDeferrer is in use. Credit to thestig@ for the test framework changes. BUG=626838 TEST=See bug for repro steps. Committed: https://crrev.com/275d69fc4e8af8ab4a8e7d8fe621ca5b408c134d Cr-Commit-Position: refs/heads/master@{#407239} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/275d69fc4e8af8ab4a8e7d8fe621ca5b408c134d Cr-Commit-Position: refs/heads/master@{#407239}
Message was sent while issue was closed.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
Message was sent while issue was closed.
I think this isn't quite right. When navigation B comes along and trashes provisional navigation A, I think we will call didStopLoading before calling UpdateNavigationState for navigation B. This kills the wrong pending_navigation_params_, and the new navigation ends up getting default values. This is causing trouble for https://codereview.chromium.org/2132603002/ where we really rely on getting accurate data for page_transition for the navigation that aborts a provisional one (navigation B in the example).
Message was sent while issue was closed.
On 2016/07/26 19:46:35, csharrison wrote: > I think this isn't quite right. > > When navigation B comes along and trashes provisional navigation A, I think we > will call didStopLoading before calling UpdateNavigationState for navigation B. > This kills the wrong pending_navigation_params_, and the new navigation ends up > getting default values. > > This is causing trouble for > https://codereview.chromium.org/2132603002/ > where we really rely on getting accurate data for page_transition for the > navigation that aborts a provisional one (navigation B in the example). Not sure how to reconcile that with the bug this was fixing, which corrupted session history. Is there some way to tell whether the params should be discarded or not? (I agree with Avi that it would be great to eventually stop storing these params as state, which has too many opportunities for bugs like this.) Can you file a bug with the details so that we can decide how to fix? Thanks!
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2190463006/ by creis@chromium.org. The reason for reverting is: This caused a regression (https://crbug.com/631617). We'll need to look for a safer way to clear the pending_navigation_params_ and try again.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
