Intent to Ship: Add value argument to URLSearchParams's has() and delete()

[Debadree Chatterjee]

Contact emails

debad...@gmail.com

Explainer

None

Specification

https://url.spec.whatwg.org/#dom-urlsearchparams-has

Summary

This feature adds the ability to pass a `value` argument to URLSearchParams's has() and delete() methods which allow for deleting tuples stored in URLSearchParams either by the `name` parameter or by the combination of `name` and `value`

Blink component

Blink

TAG review

None

TAG review status

Not applicable

Risks

Interoperability and Compatibility

Compatibility with existing websites were tested by means of a Counter in chromium, ref: https://github.com/whatwg/url/pull/735#issuecomment-1441503315 and no significant chances of breaking were found

Gecko: No signal

WebKit: No signal

Web developers: Has Shipped (https://github.com/WebKit/WebKit/pull/13500)

Other signals:

WebView application risks

Does this intent deprecate or change behavior of existing APIs, such that it has potentially high risk for Android WebView-based applications?

None expected

Debuggability

None

Will this feature be supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?

Yes

Is this feature fully tested by web-platform-tests?

Yes

Flag name

Requires code in //chrome?

False

Tracking bug

https://bugs.chromium.org/p/chromium/issues/detail?id=1442916

Estimated milestones

No milestones specified

Anticipated spec changes

Open questions about a feature may be a source of future web compat or interop issues. Please list open issues (e.g. links to known github issues in the project for the feature specification) whose resolution may introduce web compat/interop risk (e.g., changing to naming or structure of the API in a non-backward-compatible way).

None Expected

Link to entry on the Chrome Platform Status

https://chromestatus.com/feature/5147732899004416

Links to previous Intent discussions

Intent to prototype: https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CFBE060E-9D1C-4B8D-A4FB-B0279D73E6F4%40gmail.com

This intent message was generated by Chrome Platform Status.

[Debadree Chatterjee]

Given that the feature is pretty small it was recommended to me to directly take it to intent to ship stage

Thank You!

[Philip Jägenstedt]

It looks like this was spec'd in https://github.com/whatwg/url/pull/735, with participation from Chromium and WebKit folks. https://bugzilla.mozilla.org/show_bug.cgi?id=1831587 was filed for Gecko, but there's no clear position. Would you mind filing an issue at https://github.com/mozilla/standards-positions/issues/new to ensure Mozilla is aware this happening?

https://chromestatus.com/metrics/feature/timeline/popularity/4478 is pretty low, but was any analysis done of sites that reach this use counter? It's honestly higher usage than I'd expect for an argument that didn't do anything before, so likely the value passed doesn't make sense and will result in the parameter not being deleted for delete(), which could be a problem. What can you say about usage in the wild here?

+Andreu Botella 

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/A73025EA-70D7-4818-9CFF-AE14B48875F0%40gmail.com.

[Debadree Chatterjee]

Hey!

> to ensure Mozilla is aware this happening?

I am filing an issue for this

> What can you say about usage in the wild here?

In regards to this I believe no more data was collected, one example of usage where two arguments may be used was pointed out here https://github.com/whatwg/url/pull/735#issuecomment-1397898382 other than this I havent been able to come up with another example maybe it is possible to collect data on the type of argument being used in those sites? is that possible and would that be helpful here?

[Philip Jägenstedt]

On Fri, May 12, 2023 at 11:57 AM Debadree Chatterjee <debad...@gmail.com> wrote:

Hey!

> to ensure Mozilla is aware this happening?

I am filing an issue for this

Thanks! Can you link it here when filed?

 

> What can you say about usage in the wild here?

In regards to this I believe no more data was collected, one example of usage where two arguments may be used was pointed out here https://github.com/whatwg/url/pull/735#issuecomment-1397898382 other than this I havent been able to come up with another example maybe it is possible to collect data on the type of argument being used in those sites? is that possible and would that be helpful here?

It may be helpful yes, as it might reveal a usage pattern that will actually break the site in a serious way. https://chromestatus.com/metrics/feature/timeline/popularity/4478 lists 20 example URLs at the bottom of the page:

https://jornalmassa.com.br/
https://lyubercy.rus-buket.ru/
https://balashiha.rus-buket.ru/
https://astrahan.rus-buket.ru/
https://barnaul.rus-buket.ru/
https://chekhov.rus-buket.ru/
https://courses.arnos.gr/
https://www.skoleforeningen.org/
https://maisonyoko.com/
https://orel.rus-buket.ru/
https://krasnodar.rus-buket.ru/
https://ag-web.completemaths.com/
https://chelyabinsk.rus-buket.ru/
https://rus-buket.ru/
https://voronezh.rus-buket.ru/
https://wiwin.de/
https://www.arnos.gr/
https://crossfitdespentes.fr/
https://tutor.completemaths.com/

https://omsk.rus-buket.ru/

Can you go through these and see what the extra argument to has() and delete() are and what the effect of the change would be on those site? The rus-buket.ru sites are obviously a group, so checking one of them will suffice.

Best regards,

Philip 

[PhistucK]

I would imagine code like ["parameter1", "parameter2"].map(searchParameters.has.bind(searchParameters)) in which case it will start to return falses because the second parameter (index in the .map callback) will now be the value. This kind of code is always fragile, though, so kind of... Tough luck to the developer.

☆PhistucK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYfbrySi0ZuYD%2B3GpQARyZyxNasY2d3wR6L6yjFUep%2B6-A%40mail.gmail.com.

[Debadree Chatterjee]

Hey!

I have filed the Mozilla issue here https://github.com/mozilla/standards-positions/issues/801

In regards to the given urls to investigate the URLs themselves are all landing pages, I may be wrong I believe the metrics counter would be capturing usages within different user flows no? We wouldn't know what those are, any methods you suggest on how to investigate? Just went through the page source of some of the URLs the JS loaded in the landing pages themselves doesn't seem to be problematic

Yours Sincerely,

Debadree

[Andreu Botella]

jornalmassa.com.br doesn't seem to be calling these methods with two arguments, at least in my testing. The rest of sites do occasionally (sometimes with the second argument being 0, sometimes a different number, sometimes a string), but none seemed to break in my testing.

Andreu

[Philip Jägenstedt]

Hey Andreu,

Can you give an example of what the code looks like that calls the methods with a second argument?

Best regards,

Philip

[Debadree Chatterjee]

Hey!

update from mozilla's side they seem to be positive: https://github.com/mozilla/standards-positions/issues/801#issuecomment-1547693061

[PhistucK]

It would be something like this -

https://groups.google.com/a/chromium.org/g/blink-dev/c/rDaQdKpWAx8/m/qjTlRNShAgAJ

☆PhistucK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYcgT4HVYsB3tsDEpA%2B4-niBhZv5Op-ztxCi-Ae9mdAzdQ%40mail.gmail.com.

[Debadree Chatterjee]

Hey Everyone!

I am writing to seek some help regarding identifying code patterns as you say, almost all the code in these sites seems to be minified compacted so quite difficult to find manually does anyone know a better way to go about it? like is there an example of maybe modifying some code in Chrome to detect these code patterns? 

Yours Sincerely,

Debadree

[Philip Jägenstedt]

Hi Debadree, minified code is OK, all we need to see is what the call site looks like. In particular is it an explicit and intentional extra arg like `someUrl.searchParams.delete('something', extraArg)`, or is it a callback involving forEach or similar?

With some way to break in devtools at the code path in question, it should be a question of stepping up the call stack one level. The most straightforward way to do this would be a local change to Chromium, to make the methods throw an exception if given and extra argument, and then looking for the exception in devtools.

Does that sound workable? If it's unreasonably hard to do, then we need some other way to understand what the usage in the wild is about.

[PhistucK]

Excessive, but you can run this in the console -
debug(URLSearchParams.prototype.has)

debug(URLSearchParams.prototype.delete)

That will break whenever those are called.

If you want it to break less, you can replace the functions with your own and run debugger; in case there is more than one parameter -

var originalHas = URLSearchParams.prototype.has;

URLSearchParams.prototype.has = (...a) => { if (a.length > 1) debugger; return originalHas.call(this, ...a); }

var originalDelete = URLSearchParams.prototype.delete;

URLSearchParams.prototype.delete = (...a) => { if (a.length > 1) debugger; return originalDelete.call(this, ...a); }

But make sure you run this very early on the page somehow.

☆PhistucK

[Philip Jägenstedt]

Thanks PhistucK, I didn't know about these devtools tricks :D

[Debadree Chatterjee]

Hey!

Phistuck's method sounds interesting! I am giving it a try and reporting back! Thank you so much, everyone!

[PhistucK]

Yes, debug(nativeFunction) is invaluable when you want to understand what causes a JavaScript-induced navigation. :)

(Unfortunately, understanding that a simple link caused the navigation is harder)

☆PhistucK

[Debadree Chatterjee]

Is it possible to put these methods on Chrome startup? Like because loading the page would clear up devtools so in my local chromium if I could make these changes? any idea about which files would be relevant?

[PhistucK]

If you change Chromium anyway, you can just add a log/CHECK or something in the has/delete implementation itself, right?

☆PhistucK

[Debadree Chatterjee]

I mean that we have to find out how the calling function looks like so I was wondering if we could run your debug solution whenever a webpage loads, like if I could stop the page and observe what the code looks like 

[PhistucK]

If you emit a deprecation warning, I think it might show you the stack trace. How to inject stuff on page load - that is beyond my knowledge. :)

☆PhistucK

[Debadree Chatterjee]

Hey!

good news! I was able to find out a way to track this! I raised exceptions in these functions whenever they were called with two parameters (sorry it took so long 😅😅). I am attaching screenshots of what I found here:

Example Site 1: https://maisonyoko.com/

The stack trace: 

The call site:

Example Site 2: https://ag-web.completemaths.com/

The stack trace (this one seems uglified):

The call site:

Example Site 3: https://courses.arnos.gr/

The Stack Trace:

The Call Site (seems intentional?):

Example Site 4: https://wiwin.de/

The Stack Trace:

The Call Site:

Example Site 5: https://crossfitdespentes.fr/

The Stack Trace:

The Call Site:

Overall it seems PhistucK's assumption is true

Hope this is helpful! Let me know if you would like to see more examples!

Thank you!

Yours Sincerely,

Debadree

[PhistucK]

Most of them are just weird, really. I can only imagine they started with a .set with an empty string as a second parameter and ended up changing to .delete without deleting the second parameter.

(Or they had a premonition and knew there will be a second parameter with the specific purpose you want to ship hehe)

I imagine those were outliers, I would not worry much about it (also the bound callback is a bit too convoluted to be widely used), but that is just me. :)

☆PhistucK

[Andreu Botella]

As for having a premonition that this would be added, there is at least one post in the original Github issue saying that the poster already expected the two-argument overload to be supported (https://github.com/whatwg/url/issues/335#issuecomment-919700370).

Andreu

[Debadree Chatterjee]

I tried navigating and clicking around the sites, but they didn't seem to be breaking atleast even though this exception is being raised. Are there any more investigations I can do?

[Philip Jägenstedt]

Well, this is a tricky case with no obvious answer. You've found one case of array.some(...), which most likely will change the behavior of the code. For the other cases where a second argument is passed is explicitly, it depends on the value whether it changes behavior, if it's the same value that was added, then it's fine.

One concrete thing you could do is to refine the use counter to only count the cases where the 2nd argument results in has() returning false instead of true, or where delete() doesn't delete anything but would without the 2nd argument. However, I'm not sure that would be informative, if it reduces the use counter by 10x we'd still be unsure about how serious the breakage is to users.

In your manual testing of these sites, were you able to confirm the code paths were taken, and unable to spot anything at all broken on the pages? Did you compare to how the sites work without the changes?

I would say that given testing of sites that hit the code path, if you can't find anything at all breaking, then we should try to ship the change.

[Debadree Chatterjee]

For basic testing of the sites, I saw no breaking behavior, I did a few actions on sites like adding things to the cart, trying to go the login flow clicking on navigation, etc. Although I think would need to go a little deep on that, Should I submit a new CL for this counter thing? or do deeper local testing? 

[Philip Jägenstedt]

If refining the use counter is easy, that would be good to do, even if we don't block shipping on getting stable data for the use counter.

But I think that careful local testing is the best way to get a sense for the risk on this. If you're confident you've hit the code path on the sites in question, and nothing at all changes for the user, then I think we should try to ship this.

[Debadree Chatterjee]

Understood!

I am going with the local testing approach for now, I think what I will do is raise exceptions if a difference in behavior is noted as Philip suggested, and see how many of these example sites raise them. This may take a little bit of time I think but trying my best!

Thank You!

[Debadree Chatterjee]

Hey Everyone!

Sorry for the delays I followed Philip's suggestion on testing if behavior diverged in these sites, I checked this by throwing exceptions if the actual return value is different if I used name only or both name and value, I am including the code for reference:

bool URLSearchParams::has(const String& name, const String& value, ExceptionState& exception_state) const { bool found_match_name = false, found_match_name_value = false; for (const auto& param : params_) { if (param.first == name) { found_match_name = true; break; } } for (const auto& param : params_) { if (param.first == name && (value.IsNull() || param.second == value)) { found_match_name_value = true; break; } } if (found_match_name != found_match_name_value) { exception_state.ThrowException(1, "Divergent behavior"); return false; } return found_match_name_value; } void URLSearchParams::deleteAllWithNameOrTuple(const String& name, const String& value, ExceptionState& exception_state) { for (wtf_size_t i = 0; i < params_.size();) { bool match_by_name = params_[i].first == name; bool match_by_value = !value.IsNull() && params_[i].second == value; if (match_by_name) { if (match_by_value) { params_.EraseAt(i); } else { // It would have been deleted if value wasnt there exception_state.ThrowException(1, "Divergent behavior"); return; } } else { i++; } } RunUpdateSteps(); }

​

The good news is none of the example sites broke or triggered this exception at all, I navigated lightly through all the sites but no exception was observed, whereas if I raised an exception whenever double variables are passed all the sites would give an exception as you have seen in the previous mails. Do let me know if this seems correct!

Regards,

Debadree

[Philip Jägenstedt]

Hi Debadree,

That's very promising! The code looks right to me, but just to be sure, did you verify that the exceptions are thrown in a test case where the 2nd argument makes a difference? It's a bit suspicious when no sites at all threw the exception :)

Best regards,

Philip

[Debadree Chatterjee]

Hey Philip!

Even I was surprised, turns out I was wrong about the delete function (so sorry), I have observed a breakage now,

The has function output example:

The updated delete function:

```c++

void URLSearchParams::deleteAllWithNameOrTuple(const String& name,
                                               const String& value, ExceptionState& exception_state) {

  std::vector<int> indices_to_remove_with_name_val, indices_to_remove_with_name;
  for (wtf_size_t i = 0; i < params_.size(); i++) {
    if (params_[i].first == name &&
        (value.IsNull() || params_[i].second == value)) {
      indices_to_remove_with_name_val.push_back(i);
    }
  }

  for (wtf_size_t i = 0; i < params_.size(); i++) {
    if (params_[i].first == name) {
      indices_to_remove_with_name.push_back(i);
    }
  }

  if (indices_to_remove_with_name_val.size() != indices_to_remove_with_name.size()) {
    DLOG(ERROR) << "indices_to_remove_with_name_val.size() != indices_to_remove_with_name.size()";
    exception_state.ThrowException(1, "Divergent behavior");
    return;
  }

  // match the values of indices_to_remove_with_name_val, indices_to_remove_with_name
  for (size_t i = 0; i < indices_to_remove_with_name_val.size(); i++) {
    if (indices_to_remove_with_name_val[i] != indices_to_remove_with_name[i]) {
      DLOG(ERROR) << "indices_to_remove_with_name_val[i] != indices_to_remove_with_name[i]";
      exception_state.ThrowException(1, "Divergent behavior");
      return;

    }
  }

  for (wtf_size_t i = 0; i < params_.size();) {

    if (params_[i].first == name &&
        (value.IsNull() || params_[i].second == value)) {
      params_.EraseAt(i);
    } else {
      i++;
    }
  }

  RunUpdateSteps();
}

```

The example outputs of the delete function:

Example Breakages:

In https://maisonyoko.com/

https://crossfitdespentes.fr/

Other than these sites didn't notice a breakage.

seems like the breakages seem to be in the delete function only, so sorry once again for the mistake before. Do let me know if all this looks ok.

Thank You,

Debadree

[Philip Jägenstedt]

Hi Debadree,

Thank you so much for your hard work on this.

To confirm, these two sites were from the 20 listed earlier in this thread, is that right? Now that we've confirmed that these two sites will have a different behavior, can you observe any difference on https://maisonyoko.com/ or https://crossfitdespentes.fr/ with the changes, but without the exception-throwing?

Generally, finding a behavior change in 10% of tested sites is a bit concerning, but if it means that only ~10% of the cases hit by the use counter are problematic, it could be <0.001% of sites, and we've successfully made breaking changes at that level in the past.

Since you now have the code for throwing an exception, would it be straightforward to turn that into a use counter that we can land and get a better measure of this? I think as discussed previously in this thread, we should considering shipping this with a killswitch and a use counter, so we can both revert and check the usage if we get reports of breakage.

Best regards,

Philip

[Debadree Chatterjee]

Hey Philip!

In my initial testing, I didn't see any observable change in site behavior, but I shall confirm once again!

As for the kill switch and use counter should I update my existing CL to include these or make a new one containing the counter to just measure the effects?

[Debadree Chatterjee]

Hey!

So I did a more thorough testing! and it seems there is something common in both these two sites they are using the Nuxt Framework and the breakage in both of them is similar they have videos on the first section of their landing page with animations on top of them, due to the breakage it seems the video doesn't load! If you look at the call sites too the code looks very similar

for https://maisonyoko.com/

for https://crossfitdespentes.fr/

Both of them nuxt 

I am not familiar with nuxt I will try to get to know about it, but it seems like there could be some problem with nuxt, If any reader here knows nuxt would love to reach out!

Thank You

Debadree

[Mike Taylor]

I wasn't able to find anything obvious in the nuxt org on GitHub, but I did post a question in their discussion forum: https://github.com/nuxt/nuxt/discussions/21382

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/354b0282-d2c8-4ed6-9d3c-a990fda9569bn%40chromium.org.

[Philip Jägenstedt]

Hi Debadree,

I also noticed the code was similar for multiple sites, I'm glad you were able to track it down to a common framework, Nuxt. I would suggest filing an issue at https://github.com/nuxt/nuxt about this problem, maybe they can fix it in the next release. Since you've confirmed that some sites won't load the video correctly, and fixing it would require upgrading the framework, it's important to have a fix out there by the time the change is done in Chrome.

Since the WebKit change was included in STP 171, I tested https://maisonyoko.com/ and https://crossfitdespentes.fr/ in both Safari and STP 171, but I couldn't see any difference. Which video should I expect to not load as a result of this change? I was expecting something to be broken, and then I'd file a WebKit regression bug about it.

As for whether to bake the changes into a single CL or to split it, I don't have a strong opinion.

Best regards,

Philip

[Debadree Chatterjee]

Hey!

I have attached the difference in how they look for ttps://maisonyoko.com/ note that the background video doesn't load maybe since for local testing I am raising exceptions thats why its the causing issue (I am checking this) as for the nuxt issue I am still analyzing if this is a library issue or maybe its some specific style of writing nuxt

As for the CL i shall then include both the runtime flag and use counter in the same CL hopefully by tomorrow

Regards,

Debadree

[Debadree Chatterjee]

Hey!

So I tested by stopping raising the exceptions (and executing our new normal behaviour) and the behavior is still the same for https://maisonyoko.com/ do you see the same for Safari?

[Simon Lecutiez]

Hello,

> I am still analyzing if this is a library issue or maybe its some specific style of writing nuxt

Both websites were made by the same agency (Akaru), so it could even be a homemade library or a single developer’s style ;)

Hope it helps,

Simon

[Debadree Chatterjee]

Thank you so much for finding out simon!! so then nuxt is probably not to blame here 

[Debadree Chatterjee]

hello!

I checked in safari technology preview pretty weird I dont observe the same breaking behaviour, I am trying to investigate why that is so!

[Debadree Chatterjee]

Hey everyone!

sorry for the delays, I tested things locally with both Safari tech preview and my Chromium changes, and unfortunately, I see the breakage in the videos still and I don't see this happening on Safari, is it possible that my local Chromium config is to blame here? I tested my changes against the WPTs and the wpt suite seems to pass, would anyone try local testing with my CL here https://chromium-review.googlesource.com/c/chromium/src/+/4519040 to see if this reproduces for you? I have also updated the CL to include a use counter for the change so if anyone would review it would be great! PFA how it looks on my chromium

note the player error in the background.

Thank You!

Regards,

Debadree

[PhistucK]

Does it work with an unmodified Chromium (from download-chromium.appspot.com for example)?

If not, then it might just require non-free codecs, Widevine and similar.

☆PhistucK

[Philip Jägenstedt]

Indeed, Chromium isn’t built with all of the same codecs, so that’s the most likely explanation for the video not playing.

I would say that if these pages aren’t broken in Safari then that’s evidence enough that it’s not a serious failure mode.

At this point I think we know enough about the risks here.

LGTM1 to ship with the use counters added so we can get stats on how common it is if we need to revert/disable after a regression.

I would suggest re-testing these sites with Chrome Canary after the change lands to confirm no visible breakage.

[Yoav Weiss]

LGTM2

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYcWTVniD-0ny7qC7QyjnUe%2BfFrTsnvHzCC1GwCiea72Jg%40mail.gmail.com.

[Mike West]

LGTM3. Please do make sure this is behind a feature flag just in case the use counters are higher than expected.

-mike

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAL5BFfVFo8RhBT0Yn3ZZ7tyeii%2BSdXEZdJ8Jbe6bhA2QD20tZQ%40mail.gmail.com.

[Debadree Chatterjee]

Hey everyone!

Indeed the site doesn't load properly even if we use unmodified chromium as Phistuck suggested! so sorry for the confusion caused. I have updated my CL https://chromium-review.googlesource.com/c/chromium/src/+/4519040 to include both use counters and a runtime flag and am awaiting necessary approvals so we can move forward! 

Thank you!

Regards,
Debadree