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?