Impossible to display geolocation map on leboncoin.fr (error about XMLHttpRequest.withCredentials)

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Loic, Assigned: bz)

Tracking

({regression, site-compat})

45 Branch
mozilla49
regression, site-compat
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
STR: open any classified ad on the website leboncoin.fr (eg http://www.leboncoin.fr/velos/861403185.htm?ca=5_s ) and click on the orange geolocation icon to display the map in the middle of the page.

Result: only black background overlay, no map.

Reg range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c3a8eed0578&tochange=c72d38e7a212

Boris Zbarsky — Bug 814050. XMLHttpRequest.withCredentials header should throw on main thread too, in some cases. r=peterv

I think it's a TE bug. What do you think, Boris?
(Reporter)

Updated

2 years ago
Blocks: 814050
Flags: needinfo?(bzbarsky)
Keywords: regression
Are they setting withCredentials on a sync XHR and expecting that to not throw?

If so, TE indeed, though note that other browsers do seem to allow this (albeit with deprecation messages to the console), so if the site won't budge we'd just have to give up on this and get the spec changed, probably....
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 2

2 years ago
Error in the console after clicking on the icon:
Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help http://xhr.spec.whatwg.org/ http://www.leboncoin.fr/js/beta_full_54266.js:1:5195
Use of XMLHttpRequest's withCredentials attribute is no longer supported in the synchronous mode in window context. http://www.leboncoin.fr/js/beta_full_54266.js:1:5353

LeBoinCoin is pretty popular, it's the 1st French free classified ad website.
OK.  I mean, we're doing what the spec says to do.  I just tested Chrome with your steps, and the console says:

  Setting 'XMLHttpRequest.withCredentials' for synchronous requests is deprecated.

but that doesn't throw in Chrome (yet); see comment 1.

If we think the spec should change, I guess we should bring this up with the spec editor....  And probably with the Chrome folks, since their refusal to implement the spec is what's getting us here.  :(
Flags: needinfo?(annevk)

Comment 4

2 years ago
Happy to change the specification here if we decide TE doesn't work:

  https://github.com/whatwg/xhr/issues/new

Philip, any idea why Chrome has never disabled this despite the specification requiring it?
Flags: needinfo?(annevk) → needinfo?(philipj)

Comment 5

2 years ago
Let's see, it was deprecated in June 2014:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/BOgLhbdljU4

It looks like Mahesh moved on to other things and nobody else noticed or took ownership of this. Usage as fallen quite a bit but is still not a rounding error:
https://www.chromestatus.com/metrics/feature/timeline/popularity/472

Before I try to find a new owner for this or try to change it myself, why does it make sense to throw here, is sync XHR extra problematic with credentials?
Flags: needinfo?(philipj) → needinfo?(annevk)

Comment 6

2 years ago
We disabled it as part of an effort to deprecate synchronous XMLHttpRequest.
Flags: needinfo?(annevk)

Comment 7

2 years ago
OK, so in principle you could have given timeout and responseType the same treatment, but you wanted to start with something you suspect has lower usage?

Not sure what to think about this, it goes back to https://github.com/whatwg/xhr/issues/20 where I agree that getting rid of sync XHR sure would be nice, but how to actually do it I have no idea.

Is there reason to suspect that throwing here would have a leveraged effect, so that sync XHR usage would drop by more than the usage of sync XHR with credentials?

Comment 8

2 years ago
I think we started with this just after this feature was implemented. timeout and responseType existed for much longer.

I think if we offered these features synchronous XMLHttpRequest usage might increase, since there's less motivation to use asynchronous networking.

Updated

2 years ago
Keywords: site-compat

Comment 9

2 years ago
In Gecko, is this restriction artificial in the sense that it could easily be lifted? It would be artificial in Blink, and I'm really leaning towards suggesting that the spec should just allow this with sync XHR. It's the only bit of the API with this restriction, and new things are going into Fetch anyway.

The usage (~0.02% vs ~1.4%) means that it would make a very small dent in sync XHR usage, at the cost of issues like this. Sad face :(
> In Gecko, is this restriction artificial in the sense that it could easily be lifted?

Yes.

Comment 11

2 years ago
OK, so I propose to drop this restriction from spec and implementations. There's currently a deprecation message in Blink that would go away as well. Would that be acceptable, or does anyone see this as important to the overall sync XHR effort, and if so why?

Comment 12

2 years ago
Olli, thoughts on comment 11?
Flags: needinfo?(bugs)
I can live with dropping this particular restriction.
Philip, is this the only sync-XHR restriction not supported by Blink?
Flags: needinfo?(bugs) → needinfo?(philip)

Comment 14

2 years ago
The short answer is that no sync XHR restrictions are supported by Blink in that nothing throws an exception.

There's one other thing that's deprecated (but doesn't throw an exception) and that's in open(), and only on the main thread if we're not currently dispatching the beforeunload event. It's this use counter:
https://www.chromestatus.com/metrics/feature/timeline/popularity/581

I now see that the timeout and responseType setters should also throw if the sync flag is set, which I just assumed wasn't the case when writing comment #7, do these also throw in Gecko now?

Whatever the case may be, I think that only showing deprecation messages or throwing in open() would make sense, at least unless there's a reason to think that deprecating certain combinations together with XHR would have a leveraged effect in driving down usage.
Flags: needinfo?(philip)
The idea was to start throwing on all the "new features" when used with sync XHR, so that it wasn't just possible to use them. Apparently blink just failed pretty well with following the spec here.
.responseType setter does throw in Gecko when used with sync XHR in main thread, and so does .timeout setter. And open() does throw too.

Comment 16

2 years ago
Gah, again I lie because I grep for the wrong thing (deprecations instead of exceptions). Blink does throw in the timeout and responseType, and for the corresponding checks in open(). Only the withCredentials bits seems to be missing.

Looks like the last time anyone looked at the exceptions in WebKit, the spec didn't require this yet:
https://trac.webkit.org/changeset/43459
https://web.archive.org/web/20090428125543/http://dev.w3.org/2006/webapi/XMLHttpRequest-2

Given that Gecko already does this and that it's part of a group of things, I'm more uncertain about what to do. Has anyone checked the behavior of IE11 or Edge?
Edge works but puts a warning in the console: "SCRIPT7015: Setting withCredentials attribute for synchronous XMLHttpRequest is deprecated"

IE11 works and puts no warning in the console.

What do you think, Boris?
Flags: needinfo?(bzbarsky)
Whiteboard: btpp-followup-2016-04-29
I think it's worth trying to keep this if we think Blink and Edge will follow very soon.  Otherwise, I think we should give up, and just change the spec.
Flags: needinfo?(bzbarsky)

Comment 19

2 years ago
I think we should give up for the withCredentials case FWIW. Happy to change the standard.

Comment 20

2 years ago
Filed https://github.com/whatwg/xhr/issues/66
Sync XHR withCredentials is in the XHR spec now: https://github.com/whatwg/xhr/commit/90d7058031aca487f4480be07dc26cefbc4103c4

Boris, do you have time to fix this?
Flags: needinfo?(bzbarsky)
Created attachment 8748835 [details] [diff] [review]
Start allowing sync XHR with credentials in window scopes again, since no one except for us bothered to implement the spec

Tests still running, so there may be more test-only changes, I guess
Attachment #8748835 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8951085eec0f
Flags: needinfo?(bzbarsky)
Whiteboard: btpp-followup-2016-04-29 → btpp-active
Comment on attachment 8748835 [details] [diff] [review]
Start allowing sync XHR with credentials in window scopes again, since no one except for us bothered to implement the spec

oh, lovely, wpt tests required old behavior, but other browsers haven't cared of fixing their implementations.  Though, I guess that happens rather easily.
Attachment #8748835 - Flags: review?(bugs) → review+
Actually, wpt tests were inconsistent.  Some required the old behavior, some require the new one.  We still fail the latter ones because we throw the wrong exception later in the test, which is why it doesn't show up in the diff.  It's all quite lovely.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7711eb252d13

Comment 27

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/65e4abd900f4

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/65e4abd900f4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.