Bug 194329 – REGRESSION (r240909): Release assert in FrameLoader::loadURL when navigating with a non-existent target name

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Ryosuke Niwa]

After https://trac.webkit.org/changeset/240909/webkit, we're hitting the newly introduced release assertion.

Reproduction steps:
1. Navigate to: https://www.timeout.com/miami/music/rolling-loud-festival 
2. Click on the “Hard Rock Stadium” link in the article
3. Use the back button to return to the article. 
3. Click on the rollingloud.com link at the end of the article. 

<rdar://problem/47830193>

[Ryosuke Niwa]

Created attachment 361276 [details]
Fixes the bug

[Geoffrey Garen]

Comment on attachment 361276 [details]
Fixes the bug

r=me

[Daniel Bates]

Comment on attachment 361276 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=361276&action=review

> Source/WebCore/ChangeLog:12
> +        Updating the load type here should in theory fix the underlying bug r240909 was meant to catch & fix.

Just to play devil’s advocate, what makes you think you fixed it this time? I guess it’s fixed in theory. By the way, I didn’t even look at r240909, but something about this bug: its title, or this sentence. Maybe its the lack of confidence in the fix. But I’ll tell you what I love about this bug - your description (comment #0) and understandable repro steps.

[Daniel Bates]

(In reply to Daniel Bates from comment #3)
> Comment on attachment 361276 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361276&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Updating the load type here should in theory fix the underlying bug r240909 was meant to catch & fix.
> 
> Just to play devil’s advocate, what makes you think you fixed it this time?
> I guess it’s fixed in theory. By the way, I didn’t even look at r240909, but
> something about this bug: its title, or this sentence.

Something about this bug ... rubs me the wrong way.

[Ryosuke Niwa]

(In reply to Daniel Bates from comment #3)
> Comment on attachment 361276 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361276&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Updating the load type here should in theory fix the underlying bug r240909 was meant to catch & fix.
> 
> Just to play devil’s advocate, what makes you think you fixed it this time?

Because the release assertion I added in r240909 should be "comprehensive" in that either one of the release assertions is hit, or the original crash in FrameLoader::continueLoadAfterNavigationPolicy shouldn't happen.

Obviously, it's possible we read the code wrong but that's our collective understanding of the situation right now.

[Ryosuke Niwa]

Waiting for EWS.

[Ryosuke Niwa]

Committed r241015: <https://trac.webkit.org/changeset/241015>

[David Kilzer (:ddkilzer)]

(In reply to Ryosuke Niwa from comment #7)
> Committed r241015: <https://trac.webkit.org/changeset/241015>

The landed patch skipped all WebGL tests for all platforms.

I don’t think that was the intention here.

[Geoffrey Garen]

> Just to play devil’s advocate, what makes you think you fixed it this time?

I think the formal answer here is that the regression test included with this patch exercises a code path that will RELEASE_ASSERT if the bug is not fixed.

That's not my favorite way to test, but I think it's sufficient.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #8)
> (In reply to Ryosuke Niwa from comment #7)
> > Committed r241015: <https://trac.webkit.org/changeset/241015>
> 
> The landed patch skipped all WebGL tests for all platforms.
> 
> I don’t think that was the intention here.

Unskipped all webgl tests again:

Committed r241031: <https://trac.webkit.org/changeset/241031>