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.

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

Fixes #9263 #9266

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Fixes #9263 #9266

merged 2 commits into from
Mar 6, 2024

Conversation

Jarred-Sumner
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner commented Mar 5, 2024

What does this PR do?

Fixes #9263

some code copied from webkit

How did you verify your code works?

A couple tests

@Jarred-Sumner Jarred-Sumner changed the title Fixes #9283 Fixes #9263 Mar 5, 2024
@@ -101,6 +101,7 @@ void URLSearchParams::sort()
return WTF::codePointCompareLessThan(a.key, b.key);
});
updateURL();
needsSorting = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does needsSorting exist? it doesn't seem to be used

Copy link
Contributor

github-actions bot commented Mar 5, 2024

@Jarred-Sumner 1 files with test failures on bun-darwin-aarch64:

View test output

#144668befa424762643da7d046412e1edf1c22ca

Copy link
Contributor

github-actions bot commented Mar 5, 2024

@Jarred-Sumner 1 files with test failures on bun-darwin-x64:

View test output

#144668befa424762643da7d046412e1edf1c22ca

Copy link
Contributor

github-actions bot commented Mar 5, 2024

❌🪟 @Jarred-Sumner, there are 14 test regressions on Windows x86_64

  • test\cli\install\bun-upgrade.test.ts
  • test\cli\run\as-node.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\js\bun\dns\resolve-dns.test.ts
  • test\js\bun\http\fetch-file-upload.test.ts
  • test\js\bun\http\bun-server.test.ts
  • test\js\bun\shell\shelloutput.test.ts
  • test\js\bun\shell\throw.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\process\process-args.test.js
  • test\js\web\fetch\body-stream-excess.test.ts
  • test\js\web\fetch\body-stream.test.ts
  • test\js\web\timers\performance.test.js
  • test\js\web\websocket\websocket.test.js

Full Test Output

return;
m_pairs.removeAllMatching([&](const auto& pair) {
return pair.key == name && (value.isNull() || pair.value == value);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Relavent WebKit change: WebKit/WebKit@93a5678

RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.remove(WTFMove(name)); })));

String value;
EnsureStillAliveScope argument1 = callFrame->argument(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -278,7 +279,15 @@ static inline JSC::EncodedJSValue jsURLSearchParamsPrototypeFunction_deleteBody(
EnsureStillAliveScope argument0 = callFrame->uncheckedArgument(0);
auto name = convert<IDLUSVString>(*lexicalGlobalObject, argument0.value());
RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.remove(WTFMove(name)); })));

String value;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do an argsCount() check here before argument(1) since the arg count may be 1 and not 2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

argument() does the check internally

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's right, I was looking at the wrong implementation. I thought I remembered something about uncheckedArgument: https://github.com/WebKit/WebKit/blob/de89f2977a314e5ddbe49384e35b953061d17f29/Source/JavaScriptCore/interpreter/CallFrame.h#L275-L285

@Jarred-Sumner Jarred-Sumner merged commit edeeffc into main Mar 6, 2024
25 of 30 checks passed
@Jarred-Sumner Jarred-Sumner deleted the jarred/fixes-9263 branch March 6, 2024 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URLSearchParams.prototype.{ has, delete } second argument support
3 participants