Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix assertion for popup about:blank navigations without WKNavigation #247

Merged
merged 2 commits into from
Mar 9, 2023
Merged

fix assertion for popup about:blank navigations without WKNavigation #247

merged 2 commits into from
Mar 9, 2023

Conversation

mallexxx
Copy link
Collaborator

@mallexxx mallexxx commented Mar 2, 2023

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/1177771139624306/1204065038916789/f
iOS PR: - not affected
macOS PR: duckduckgo/macos-browser#1008
What kind of version bump will this require?: Patch

Optional:

Steps to test this PR:

  1. window.open("") from js console, validate no assertion is raised

@mallexxx mallexxx mentioned this pull request Mar 2, 2023
Merged
ayoy
ayoy requested changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@ayoy ayoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @mallexxx! I’ve found that even with this patch, opening settings from Duck Player (via ⚙️ icon) crashes on the assertion. Please have a look when you’re back. Thanks again.

Sources/Navigation/DistributedNavigationDelegate.swift
@@ -696,7 +698,7 @@ extension DistributedNavigationDelegate: WKNavigationDelegate {
@MainActor
public func webView(_ webView: WKWebView, didCommit wkNavigation: WKNavigation?) {
guard let navigation = wkNavigation?.navigation ?? startedNavigation else {
assertionFailure("Unexpected didCommitNavigation")
assert(wkNavigation == nil, "Unexpected didCommitNavigation without preceding didStart")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos for the more descriptive assertion message!

Sources/Navigation/DistributedNavigationDelegate.swift
@@ -416,10 +416,12 @@ extension DistributedNavigationDelegate: WKNavigationDelegate {
if let expectedNavigation = navigationExpectedToStart, wkNavigation != nil || expectedNavigation.navigationAction.navigationType == .sessionRestoration {
// regular flow: start .expected navigation
navigation = expectedNavigation
} else {
} else if webView.url?.isEmpty == false {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it fixes calling window.open(“”), it doesn’t seem to handle window.open(“about:settings”) which is used in Duck Player to open settings and as such causes an assertion on debug builds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@ayoy ayoy assigned mallexxx and unassigned ayoy Mar 8, 2023
@mallexxx mallexxx assigned ayoy and unassigned mallexxx Mar 9, 2023
ayoy
ayoy approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@ayoy ayoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@ayoy ayoy assigned mallexxx and unassigned ayoy Mar 9, 2023
@mallexxx mallexxx merged commit c38de54 into main Mar 9, 2023
@mallexxx mallexxx deleted the alex/fix-did-start-assertion branch March 9, 2023 12:08
mallexxx added a commit to duckduckgo/macos-browser that referenced this pull request Mar 9, 2023
@mallexxx
localhost/about:blank Permission fixes (#1008)
d45ce5e 
Task/Issue URL: 
- https://app.asana.com/0/1177771139624306/1204065038916781/f
- https://app.asana.com/0/1177771139624306/1204065038916785/f
- https://app.asana.com/0/1177771139624306/1204065038916789/f
BSK PR: duckduckgo/BrowserServicesKit#247

**Description**:
- Fixes popup opened with empty url (window.open("")) asserting
- Fixes Empty URL being shown in Popup Permissions menu as an empty item
- Fixes permission requests for file:// urls asserting, sets "localhost"
as a domain name

**Steps to test this PR**:
1. window.open("") from js console on an empty page, validate popup
opens instantly
2. window.open("") from js console on a file://, validate permission
request uses "localhost" domain and shows “” as popup url
3. Validate other permissions use "localhost" for file:// urls and not
asserting

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
**When ready for review, remember to post the PR in MM**
ayoy pushed a commit to duckduckgo/macos-browser that referenced this pull request Mar 10, 2023
@mallexxx @ayoy
localhost/about:blank Permission fixes (#1008)
a375240 
Task/Issue URL: 
- https://app.asana.com/0/1177771139624306/1204065038916781/f
- https://app.asana.com/0/1177771139624306/1204065038916785/f
- https://app.asana.com/0/1177771139624306/1204065038916789/f
BSK PR: duckduckgo/BrowserServicesKit#247

**Description**:
- Fixes popup opened with empty url (window.open("")) asserting
- Fixes Empty URL being shown in Popup Permissions menu as an empty item
- Fixes permission requests for file:// urls asserting, sets "localhost"
as a domain name

**Steps to test this PR**:
1. window.open("") from js console on an empty page, validate popup
opens instantly
2. window.open("") from js console on a file://, validate permission
request uses "localhost" domain and shows “” as popup url
3. Validate other permissions use "localhost" for file:// urls and not
asserting

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
**When ready for review, remember to post the PR in MM**
samsymons added a commit that referenced this pull request Apr 4, 2023
@samsymons
Merge branch 'main' into sam/swiftlint-plugin
f98fa7c 
# By Alexey Martemyanov (8) and others
# Via GitHub
* main:
  Make FeatureName a struct so it can be extended from client code (#276)
  add count of all bookmarks in domain to view model (#272)
  Configuration updates optimizations (#269)
  Roll-back InteractiveUserScript (#268)
  Update C-S-S to 4.4.4 (#260)
  Add invalid payload pixel handler (#264)
  Fix Error pixel misfiring on valid scenario (#261)
  add didCancelNavigationAction (#262)
  Opt-outable private API usage (#254)
  HTTPS upgrade threading (#256)
  DDGSync module (#253)
  fix delayed new window with empty url creation handling (#255)
  remove major tracker network (#251)
  Enable username&password passed in URL (#245)
  fix assertion for popup about:blank navigations without WKNavigation (#247)
  fix about: scheme fragments dropping (#246)
  Unify Configuration Management (#241)

# Conflicts:
#	Package.resolved
#	Package.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ayoy ayoy ayoy approved these changes

@brindy brindy Awaiting requested review from brindy

@bwaresiak bwaresiak Awaiting requested review from bwaresiak

@THISISDINOSAUR THISISDINOSAUR Awaiting requested review from THISISDINOSAUR

@tomasstrba tomasstrba Awaiting requested review from tomasstrba

@samsymons samsymons Awaiting requested review from samsymons

Assignees

@mallexxx mallexxx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mallexxx @ayoy