Bug 189190

Summary: REGRESSION(r235398) ASSERTION failure !m_client.didFinishDocumentLoadForFrame
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: WebKit2Assignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, cdumez, commit-queue, darin, dbates, mitz, rwlbuis, thorton, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=189234
Attachments:
Description Flags
Patch
none
Patch
none
patch none

Frédéric Wang (:fredw)
Reported 2018-08-31 05:36:39 PDT
There are assertions in the deprecated WKPageSetPageLoaderClient class after bug 188997 ; it seems they are currently hit when running build with the external SDK.
Attachments
Patch (1.98 KB, patch)
2018-08-31 05:53 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (2.19 KB, patch)
2018-09-03 01:39 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
patch (2.13 KB, patch)
2018-09-04 09:50 PDT, Alex Christensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2018-08-31 05:53:48 PDT
Created attachment 348636 [details] Patch
Alex Christensen
Comment 2 2018-08-31 09:59:31 PDT
I assume this is when you run system safari with your build of WebKit. Does Safari work well enough with this WebKit?
Alex Christensen
Comment 3 2018-08-31 10:01:23 PDT
This would be more correct if it were a version check to see if you're on Mojave or before.
Alex Christensen
Comment 4 2018-08-31 10:19:56 PDT
Comment on attachment 348636 [details] Patch I mean, we would want to not assert if you're on Mojave or before just to work with system safari.
Frédéric Wang (:fredw)
Comment 5 2018-08-31 10:32:01 PDT
(In reply to Alex Christensen from comment #2) > I assume this is when you run system safari with your build of WebKit. Does > Safari work well enough with this WebKit? I'm just doing run-safari [--ios-simulator] using the public SDK, so AFAIK yes it is the system one. (In reply to Alex Christensen from comment #3) > This would be more correct if it were a version check to see if you're on > Mojave or before. Right, I'll try to see how to do that.
Alex Christensen
Comment 6 2018-08-31 10:32:53 PDT
Also, if you could get it to use Safari Tech Preview instead of Safari, that would probably fix it.
Alexey Proskuryakov
Comment 7 2018-09-01 19:47:18 PDT
Is "Mojave or before" the right check, or is it actually about Safari 11.x?
Frédéric Wang (:fredw)
Comment 8 2018-09-03 01:19:30 PDT
(In reply to Alex Christensen from comment #6) > Also, if you could get it to use Safari Tech Preview instead of Safari, that > would probably fix it. I think I've never been able to make run-webkit-app work ; I just tried again and Safari Tech Preview does not seem to use the local build. Also, in any case that's not an option for running Safari on the iOS simulator. (In reply to Alexey Proskuryakov from comment #7) > Is "Mojave or before" the right check, or is it actually about Safari 11.x? Good question. Comment 6 seems to suggest it's about the Safari version, not the OS so probably we should try that.
Frédéric Wang (:fredw)
Comment 9 2018-09-03 01:34:19 PDT
*** Bug 189233 has been marked as a duplicate of this bug. ***
Frédéric Wang (:fredw)
Comment 10 2018-09-03 01:39:34 PDT
Created attachment 348762 [details] Patch
Frédéric Wang (:fredw)
Comment 11 2018-09-03 01:40:49 PDT
I've updated the patch to use a OS version check (I'm not sure how to check Safari's version). I hope it's good enough.
Alex Christensen
Comment 12 2018-09-04 09:50:23 PDT
Created attachment 348822 [details] patch
Alex Christensen
Comment 13 2018-09-04 09:53:17 PDT
I made the check for Mojave or before rather than add a bundle check for Safari. That should be good enough for the open source developers checking their work with Safari until the next release.
WebKit Commit Bot
Comment 14 2018-09-04 10:28:36 PDT
Comment on attachment 348822 [details] patch Clearing flags on attachment: 348822 Committed r235618: <https://trac.webkit.org/changeset/235618>
WebKit Commit Bot
Comment 15 2018-09-04 10:28:38 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2018-09-04 10:29:19 PDT
<rdar://problem/44102173>
Frédéric Wang (:fredw)
Comment 17 2018-09-04 11:10:46 PDT
(In reply to WebKit Commit Bot from comment #14) > Comment on attachment 348822 [details] > patch > > Clearing flags on attachment: 348822 > > Committed r235618: <https://trac.webkit.org/changeset/235618> @Alex: Any reason why you removed the condition "PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000"? It seems the landed commit will still fail to make "run-safari --ios-simulator" work for open source developers...
Alex Christensen
Comment 18 2018-09-04 11:11:52 PDT
iOS Safari never used WKPageSetPageLoaderClient :)
Frédéric Wang (:fredw)
Comment 19 2018-09-04 11:32:23 PDT
OK, maybe I mixed up the other day because bug 189188 happened at the same time.
Note You need to log in before you can comment on or make changes to this bug.