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

393 views
Skip to first unread message

Debadree Chatterjee

unread,
May 12, 2023, 3:30:08 AM5/12/23
to blin...@chromium.org

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

unread,
May 12, 2023, 3:32:50 AM5/12/23
to blink-dev, 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

unread,
May 12, 2023, 4:39:21 AM5/12/23
to Debadree Chatterjee, Andreu Botella, blin...@chromium.org
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?


--
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

unread,
May 12, 2023, 5:57:07 AM5/12/23
to blink-dev, Philip Jägenstedt, blin...@chromium.org, Debadree Chatterjee, abot...@igalia.com
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

unread,
May 12, 2023, 7:02:12 AM5/12/23
to Debadree Chatterjee, blink-dev, abot...@igalia.com
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?

PhistucK

unread,
May 12, 2023, 8:18:33 AM5/12/23
to Philip Jägenstedt, Debadree Chatterjee, Andreu Botella, blin...@chromium.org
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


Debadree Chatterjee

unread,
May 12, 2023, 1:43:39 PM5/12/23
to blink-dev, PhistucK, Debadree Chatterjee, abot...@igalia.com, blin...@chromium.org, Philip Jägenstedt
Hey!


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

unread,
May 12, 2023, 2:51:27 PM5/12/23
to Philip Jägenstedt, Debadree Chatterjee, blin...@chromium.org

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

unread,
May 15, 2023, 5:25:45 AM5/15/23
to Andreu Botella, Debadree Chatterjee, blin...@chromium.org
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

unread,
May 15, 2023, 9:21:00 AM5/15/23
to blink-dev, Philip Jägenstedt, Debadree Chatterjee, blin...@chromium.org, abot...@igalia.com
Hey!

PhistucK

unread,
May 15, 2023, 12:36:05 PM5/15/23
to Philip Jägenstedt, Andreu Botella, Debadree Chatterjee, blin...@chromium.org

Debadree Chatterjee

unread,
May 17, 2023, 11:27:34 AM5/17/23
to blink-dev, PhistucK, abot...@igalia.com, Debadree Chatterjee, blin...@chromium.org, Philip Jägenstedt
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

unread,
May 17, 2023, 12:17:30 PM5/17/23
to Debadree Chatterjee, blink-dev, PhistucK, abot...@igalia.com
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

unread,
May 17, 2023, 12:31:41 PM5/17/23
to Philip Jägenstedt, Debadree Chatterjee, blink-dev, abot...@igalia.com
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

unread,
May 17, 2023, 12:39:10 PM5/17/23
to PhistucK, Debadree Chatterjee, blink-dev, abot...@igalia.com
Thanks PhistucK, I didn't know about these devtools tricks :D

Debadree Chatterjee

unread,
May 17, 2023, 12:41:51 PM5/17/23
to blink-dev, Philip Jägenstedt, Debadree Chatterjee, blink-dev, abot...@igalia.com, PhistucK
Hey!

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

PhistucK

unread,
May 17, 2023, 12:46:08 PM5/17/23
to Philip Jägenstedt, Debadree Chatterjee, blink-dev, abot...@igalia.com
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

unread,
May 17, 2023, 12:50:07 PM5/17/23
to blink-dev, Debadree Chatterjee, Philip Jägenstedt, blink-dev, abot...@igalia.com, PhistucK
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

unread,
May 17, 2023, 2:46:26 PM5/17/23
to Debadree Chatterjee, blink-dev, Philip Jägenstedt, abot...@igalia.com
If you change Chromium anyway, you can just add a log/CHECK or something in the has/delete implementation itself, right?

PhistucK

Debadree Chatterjee

unread,
May 17, 2023, 2:50:55 PM5/17/23
to blink-dev, PhistucK, blink-dev, Philip Jägenstedt, abot...@igalia.com, 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

unread,
May 17, 2023, 3:15:04 PM5/17/23
to Debadree Chatterjee, blink-dev, Philip Jägenstedt, abot...@igalia.com
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

unread,
May 18, 2023, 4:42:32 PM5/18/23
to blink-dev, PhistucK, blink-dev, Philip Jägenstedt, abot...@igalia.com, 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: 
Screenshot 2023-05-19 at 1.51.18 AM.png
The call site:
Screenshot 2023-05-19 at 1.52.45 AM.png

The stack trace (this one seems uglified):
Screenshot 2023-05-19 at 1.54.00 AM.png
The call site:
Screenshot 2023-05-19 at 1.55.59 AM.png

Example Site 3: https://courses.arnos.gr/
The Stack Trace:
Screenshot 2023-05-19 at 1.57.56 AM.png
The Call Site (seems intentional?):
Screenshot 2023-05-19 at 1.58.54 AM.png

Example Site 4: https://wiwin.de/
The Stack Trace:
Screenshot 2023-05-19 at 2.00.48 AM.png
The Call Site:
Screenshot 2023-05-19 at 2.01.21 AM.png

The Stack Trace:
Screenshot 2023-05-19 at 2.08.40 AM.png
The Call Site:
Screenshot 2023-05-19 at 2.09.33 AM.png

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

unread,
May 18, 2023, 5:43:00 PM5/18/23
to Debadree Chatterjee, blink-dev, Philip Jägenstedt, abot...@igalia.com
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

unread,
May 18, 2023, 6:29:21 PM5/18/23
to PhistucK, Debadree Chatterjee, blink-dev, Philip Jägenstedt

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

unread,
May 19, 2023, 9:40:49 AM5/19/23
to blink-dev, abot...@igalia.com, blink-dev, Philip Jägenstedt, PhistucK, 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

unread,
May 22, 2023, 12:39:26 PM5/22/23
to Debadree Chatterjee, blink-dev, abot...@igalia.com, PhistucK
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

unread,
May 22, 2023, 12:59:18 PM5/22/23
to blink-dev, Philip Jägenstedt, blink-dev, abot...@igalia.com, PhistucK, 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

unread,
May 24, 2023, 11:43:04 AM5/24/23
to Debadree Chatterjee, blink-dev, abot...@igalia.com, PhistucK
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

unread,
May 24, 2023, 1:05:59 PM5/24/23
to blink-dev, Philip Jägenstedt, blink-dev, abot...@igalia.com, PhistucK, 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

unread,
May 30, 2023, 5:45:19 AM5/30/23
to blink-dev, Debadree Chatterjee, Philip Jägenstedt, blink-dev, abot...@igalia.com, PhistucK
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

unread,
May 30, 2023, 8:04:18 AM5/30/23
to Debadree Chatterjee, blink-dev, abot...@igalia.com, PhistucK
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

unread,
May 30, 2023, 11:16:01 AM5/30/23
to blink-dev, Philip Jägenstedt, blink-dev, abot...@igalia.com, PhistucK, 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:
Screenshot 2023-05-30 at 7.36.36 PM.png


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:
Screenshot 2023-05-30 at 8.39.03 PM.png

Example Breakages:
Screenshot 2023-05-30 at 8.33.59 PM.png
Screenshot 2023-05-30 at 8.42.01 PM.png
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

unread,
May 31, 2023, 11:58:39 AM5/31/23
to Debadree Chatterjee, blink-dev, abot...@igalia.com, PhistucK
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

unread,
May 31, 2023, 12:08:18 PM5/31/23
to blink-dev, Philip Jägenstedt, blink-dev, abot...@igalia.com, PhistucK, 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?