NavigationHandle::GetGlobalRequestID() returns uninitialized values when browser side nav enabled |
||||
Issue descriptionI'm working on a change that uses the GlobalRequestID returned by NavigationHandle::GetGlobalRequestID(). My code DCHECKs that this method returns a GlobalRequestId with child_id and request_id values >= 0. These DCHECKs pass consistently when browser side navigation is not enabled, but fail consistently when browser side navigation is enabled. For now, I've guarded this part of my code in a call to !IsBrowserSideNavigationEnabled() to avoid the DCHECKs firing, but this means my code doesn't work at all when browser side navigation is enabled. See https://codereview.chromium.org/2624283004 for the relevant change (see PageLoadTracker::ReadyToCommit for the specific code that's affected).
,
Jan 13 2017
Comment #1 is incorrect; my original DCHECK had a bug in that it expected that the child_id field was always >= -1, however I've since modified the DCHECK to: DCHECK(navigation_handle->GetGlobalRequestID() != content::GlobalRequestID()); and I see this firing in WebContentsObserver::ReadyToCommitNavigation when browser side navigation is enabled. So, there appears to be a real bug here.
,
Jan 13 2017
Do you have a repro for this? Does a specific test hit this DCHECK? I wonder if it is being hit in cases where we don't make network requests, which obviously won't have a GlobalRequestID attached to it.
,
Jan 17 2017
,
Jan 18 2017
Here's a minimal change that shows how this causes many browser tests to fail: https://codereview.chromium.org/2635193004 The list of failing tests can be seen here: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/372601/steps/browser_side_navigation_unit_tests%20%28with%20patch%29
,
Jan 18 2017
All of the failing tests that I looked at from the list in comment 5 have content::TestNavigationURLLoader::CallOnResponseStarted on the stack, which passes in a default GlobalRequestID. bmcquade, were you also seeing this in chrome use or just running tests with plznavigate enabled? Can you add a non-default global request id instead (e.g., 1,1) for the test code and see if that fixes the issues: https://cs.chromium.org/chromium/src/content/test/test_navigation_url_loader.cc?sq=package:chromium&dr=CSs&rcl=1484684504&l=59
,
Jan 18 2017
Ah, interesting, thanks! Yes I think that's a reasonably sane change to make. Nasko, does passing a non default GlobalRequestID in the tests as Ryan suggests sound ok to you?
,
May 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50996ef01397fc67caece4810a2c51f91f926cdb commit 50996ef01397fc67caece4810a2c51f91f926cdb Author: csharrison <csharrison@chromium.org> Date: Tue May 16 14:05:34 2017 NavigationSimulator: add support for GlobalRequestIds This adds support for both the PlzNavigate and non-PlzNavigate paths, and ensures that TestNavigationURLLoader always uses a non default one. BUG= 680841 , 711352 Review-Url: https://codereview.chromium.org/2885453003 Cr-Commit-Position: refs/heads/master@{#472093} [modify] https://crrev.com/50996ef01397fc67caece4810a2c51f91f926cdb/chrome/browser/page_load_metrics/page_load_tracker.cc [modify] https://crrev.com/50996ef01397fc67caece4810a2c51f91f926cdb/content/public/test/navigation_simulator.cc [modify] https://crrev.com/50996ef01397fc67caece4810a2c51f91f926cdb/content/test/test_navigation_url_loader.cc
,
May 16 2017
I think this is fixed with the above change. Bryan, please reopen if I'm being premature. |
||||
►
Sign in to add a comment |
||||
Comment 1 by bmcquade@chromium.org
, Jan 13 2017