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

Issue 671176 link

Starred by 1 user
Status: WontFix
Owner: ----
Closed: Dec 2016
Cc:
clamy@chromium.org
pbomm...@chromium.org
█ nasko@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Assertion failure triggered by TabSpecificContentSettings::DidStartNavigation()

Project Member Reported by wdzierza...@opera.com, Dec 5 2016 
Chrome Version: 55.0.2883.75
OS: Any

It seems https://codereview.chromium.org/2524053002, when merged to the 55 branch -- causes an assertion failure in https://chromium.googlesource.com/chromium/src/+/ea46662d4008c9fcfb8a3ee1933dd2f5e90a9c3b/content/browser/frame_host/navigation_handle_impl.cc#208 every time.

IsSamePage() must not be called before DidCommitNavigation(), but that CL adds an IsSamePage() call to DidStartNavigation(). "start" happens before "commit"...

IIUC, this problem has already been identified in https://bugs.chromium.org/p/chromium/issues/detail?id=667256#c32, but the assertion itself has not been addressed.

Note that this problem doesn't occur on master, most likely thanks to https://codereview.chromium.org/2345053006.

Comment 1 by clamy@chromium.org, Dec 5 2016

Cc: nasko@chromium.org
Status: WontFix (was: Untriaged)
+nasko

On trunk, Nasko's patch has removed the assertion, so it is not a problem.

On M55, the assertion is a DCHECK, so this should not be causing issues to users in the field. Since M55 has been released and it is not problematic for users, it seems too late to address this and do a merge to the M55 branch. Since the issue has already been adressed for versions following M55, I'm closing the bug.

Comment 2 by wdzierza...@opera.com, Dec 5 2016 

Hmm, right, being a DCHECK, it won't just crash the browser.  However, the call to IsSamePage() does seem to happen too early and thus give potentially incorrect results.

Note that https://codereview.chromium.org/2345053006 didn't just remove the assertion.  Instead, it changed the implementation so that |is_same_page_| is properly initialized in the constructor and thus IsSamePage() can be called any time.

Comment 3 by clamy@chromium.org, Dec 5 2016 

Indeed it was giving the wrong value. This is why we later merged https://codereview.chromium.org/2536423003/ which uses IsSynchronous, (which was removed by Nasko's patch). IsSynchronous returns the right value in the M55 context. I reckon that being so close to release, I did not revert the merge of https://codereview.chromium.org/2524053002 on the release branch.

Comment 4 by wdzierza...@opera.com, Dec 5 2016 

Ah, now I see what happened.  Thanks a lot for taking the time to clarify!
►

Sign in to add a comment