Bug 194329 - REGRESSION (r240909): Release assert in FrameLoader::loadURL when navigating with a non-existent target name
Summary: REGRESSION (r240909): Release assert in FrameLoader::loadURL when navigating ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar, Regression
Depends on: 194189
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-05 20:59 PST by Ryosuke Niwa
Modified: 2019-02-06 10:59 PST (History)
10 users (show)

See Also:


Attachments
Fixes the bug (5.50 KB, patch)
2019-02-05 21:06 PST, Ryosuke Niwa
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-02-05 20:59:03 PST
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>
Comment 1 Ryosuke Niwa 2019-02-05 21:06:01 PST
Created attachment 361276 [details]
Fixes the bug
Comment 2 Geoffrey Garen 2019-02-05 21:17:24 PST
Comment on attachment 361276 [details]
Fixes the bug

r=me
Comment 3 Daniel Bates 2019-02-05 21:59:05 PST
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.
Comment 4 Daniel Bates 2019-02-05 22:00:31 PST
(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.
Comment 5 Ryosuke Niwa 2019-02-05 22:05:51 PST
(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.
Comment 6 Ryosuke Niwa 2019-02-05 22:06:33 PST
Waiting for EWS.
Comment 7 Ryosuke Niwa 2019-02-06 01:42:51 PST
Committed r241015: <https://trac.webkit.org/changeset/241015>
Comment 8 David Kilzer (:ddkilzer) 2019-02-06 02:11:50 PST
(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.
Comment 9 Geoffrey Garen 2019-02-06 10:44:20 PST
> 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.
Comment 10 David Kilzer (:ddkilzer) 2019-02-06 10:59:56 PST
(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>