Last Comment Bug 679971 - remove Navigator.taintEnabled()
: remove Navigator.taintEnabled()
Status: REOPENED
[qa-]
: addon-compat, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
-- normal with 3 votes (vote)
: ---
Assigned To: Matheus Kerschbaum
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 683151 695337 711955 712805 712949 714740 717558 719322 726091 732131
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-17 19:22 PDT by Andreas Gal :gal
Modified: 2015-10-28 06:50 PDT (History)
33 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+
+
-


Attachments
patch (2.20 KB, patch)
2011-08-17 20:23 PDT, Matheus Kerschbaum
gal: review+
Details | Diff | Splinter Review
backout patch (5.45 KB, patch)
2011-12-22 16:10 PST, Olli Pettay [:smaug]
bzbarsky: review+
jst: review+
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review
for aurora (2.19 KB, patch)
2011-12-29 15:12 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
for beta (2.02 KB, patch)
2011-12-29 15:13 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description User image Andreas Gal :gal 2011-08-17 19:22:21 PDT
It returns false and has been unused in probably a decade.
Comment 1 User image Matheus Kerschbaum 2011-08-17 20:23:06 PDT
Created attachment 553991 [details] [diff] [review]
patch
Comment 2 User image Brendan Eich [:brendan] 2011-08-18 11:11:42 PDT
A decade ago was 2001. Netscape 3's experimental (opt-in via envar) data tainting security model was not carried into Nescape 4, so more like 14 years.

/be
Comment 3 User image Andreas Gal :gal 2011-08-18 11:15:18 PDT
Comment on attachment 553991 [details] [diff] [review]
patch

Stealing. Lets land it early in the cycle and watch for web compatibility regressions.
Comment 4 User image Ed Morley [:emorley] 2011-08-20 15:57:56 PDT
Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=626e32292fdc

Assuming all green, will push to inbound after :-)
Comment 5 User image Ed Morley [:emorley] 2011-08-20 16:05:14 PDT
(In case anyone pushes this to inbound before me, the r= needs correcting to =gal, rather than bz)
Comment 7 User image :Ms2ger (⌚ UTC+1/+2) 2011-08-21 11:26:39 PDT
http://hg.mozilla.org/mozilla-central/rev/857f058efa56
Comment 8 User image Eric Shepherd [:sheppy] 2011-08-22 14:04:42 PDT
Docs updated.
Comment 9 User image Jorge Villalobos [:jorgev] 2011-09-29 17:30:58 PDT
And yet some scripts use it, apparently for browser detection.
Comment 10 User image j.j. 2011-10-18 10:36:05 PDT
It's documented in
  https://developer.mozilla.org/en/Firefox_8_for_developers
but "Target Milestone" is Mozilla 9.
What's right?
Comment 11 User image j.j. 2011-10-18 10:41:03 PDT
The release calendar says "Mozilla 9".
Adding "dev-doc-needed" keyword to clarify.
Comment 12 User image Eric Shepherd [:sheppy] 2011-10-18 11:04:06 PDT
Note moved to Firefox 9 for developers.
Comment 13 User image j.j. 2011-10-18 14:14:24 PDT
This breaks some older MediaWiki installations (bug 695337 and bug 683151). Probably enough to backout for now?!
Comment 14 User image Andreas Gal :gal 2011-10-20 19:51:30 PDT
This is not a high value patch, so I am not opposed to backing out, but it would be nice to retire this and I don't see any way doing that thats not a little bit painful and involves evangelism to get sites to stop using it.
Comment 15 User image j.j. 2011-10-22 05:27:54 PDT
> I don't see any way doing that thats not a little bit painful
> and involves evangelism to get sites to stop using it.

Add a fat warning, wait some month and remove it then would at least give them a chance to realize it before last-minute.
(And would increase probability that old MediaWiki pages had updated anyway meanwhile) 

Note these bug reports are for Auroa. Would expect much more for Beta and final release.
Comment 16 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-23 14:02:02 PDT
Our experience is generally that people just ignore the warnings.  The only value that they add is us being able to say "hey, we warned you".
Comment 17 User image j.j. 2011-10-23 19:55:20 PDT
> Our experience is generally that people just ignore the warnings
I'v seen other opinions (eg bug 585877 comment 23)
Comment 18 User image :Ms2ger (⌚ UTC+1/+2) 2011-10-24 05:02:30 PDT
Not my experience either; facebook stopped using getAttributeNode shortly after we started to warn about it, for example.
Comment 19 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-07 13:18:30 PST
Code cleanup, QA doesn't need to verify this fix.
Comment 20 User image j.j. 2011-12-19 12:32:50 PST
Why the hell this wasn't this backed out for at least one year or so? It strikes back now:

At least jquery 1.6.2 uses  navigator.taintEnabled  for browser detection, see bug 711955.
Comment 21 User image j.j. 2011-12-19 12:36:14 PST
[take the tracking-firefox9 flag and stick it on your Christmas tree]
Comment 22 User image j.j. 2011-12-19 18:04:37 PST
> At least jquery 1.6.2 uses  navigator.taintEnabled

That was wrong. It is "jQuery UI 1.8.4"
Comment 23 User image j.j. 2011-12-21 18:14:20 PST
Probably breaks a top 10 page in Poland (http://nk.pl/)
Comment 24 User image Andreas Gal :gal 2011-12-21 18:23:07 PST
As stupid this is, we might have to punt on this removal considering the web compatibility implications. Shame on jquery, seriously.
Comment 25 User image Andreas Gal :gal 2011-12-21 18:27:51 PST
I filed http://bugs.jquery.com/ticket/11095 for this.
Comment 26 User image j.j. 2011-12-21 18:29:05 PST
> Shame on jquery, seriously.
The nk.pl case is not jquery UI, might be MooTools (and page breaks completely)
Comment 27 User image j.j. 2011-12-21 18:32:48 PST
(In reply to Andreas Gal :gal from comment #25)
> I filed http://bugs.jquery.com/ticket/11095 for this.

The bug is in jquery UI 1.8.4, thats a different library than jquery. Bug tracker is here:
  http://bugs.jqueryui.com/

Might be fixed in jquery UI 1.8.6
Comment 28 User image j.j. 2011-12-21 18:59:34 PST
(In reply to j.j. from comment #26)
> The nk.pl case is not jquery UI, might be MooTools

The browser detection with "taintEnabled()" is not in the MooTools part, it's own code of nk.pl
Comment 29 User image Alex Keybl [:akeybl] 2011-12-21 22:34:53 PST
Is there any possibility for a workaround that can be communicated through evangelism to the top 10 Polish site nk.pl? See bug 712805 for more background.
Comment 30 User image :Ehsan Akhgari 2011-12-22 10:37:51 PST
A simple codesearch query seems to suggest that this is a pretty popular UA sniffing tool unfortunately <http://www.google.com/codesearch#search/&q=navigator.taintEnabled&type=cs>.

We could potentially enable checks for existence of this function but prevent scripts from calling it for a while, but I suspect that the required evangelism effort might not justify the gain.  :(
Comment 31 User image Zibi Braniecki [:gandalf][:zibi] 2011-12-22 13:26:43 PST
the nk.pl has been fixed today, but they're still detecting browsers basing on this attribute.

If we can get MooTools and jQuery UI to fix this we should be good. I can't find any other top website that would be affected by this change.
Comment 32 User image Marek Stępień [:marcoos, inactive] 2011-12-22 13:34:30 PST
Current MooTools 1.4.2 don't use taintEnabled. Some older version did, though.
Comment 33 User image Olli Pettay [:smaug] 2011-12-22 14:20:19 PST
If we decide to back this out, some one needs to prepare a patch which has a new interface which
nsNavigator implements. The interface would contain only taintEnabled(). That way we don't break
any binary addons.
The interface name could be hopefully something like nsINavigator_9_0_2.

Anyone willing to prepare such patch. (I would do it, but unfortunately I'm in a place with
unreliable network connection.)
Comment 34 User image Marek Stępień [:marcoos, inactive] 2011-12-22 14:58:35 PST
Another class of sites that get broken due to taintEnabled used as a lame way to distinguish between Gecko and WebKit are pre-1.16 MediaWiki instances such as in bug 695337 and bug 683151. An example of a popular site like this would be www.wikitravel.org.
Comment 35 User image Olli Pettay [:smaug] 2011-12-22 16:10:00 PST
Created attachment 583959 [details] [diff] [review]
backout patch
Comment 36 User image Zibi Braniecki [:gandalf][:zibi] 2011-12-22 16:15:22 PST
Don't we want to add a warning?
Comment 37 User image j.j. 2011-12-22 16:19:37 PST
> are pre-1.16 MediaWiki instances
That's the crux with this bug, known since October (see comment 13)
Comment 38 User image christian 2011-12-29 13:50:21 PST
Comment on attachment 583959 [details] [diff] [review]
backout patch

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Older jquery and mediawiki installations will stay broken
Testing completed (on m-c, etc.): Not tested
Risk to taking this patch (and alternatives if risky): Doesn't look too risky, adds a new interface to not break binary add-ons
Comment 39 User image :Ehsan Akhgari 2011-12-29 14:26:00 PST
(In reply to Zbigniew Braniecki [:gandalf] from comment #36)
> Don't we want to add a warning?

I think we do.
Comment 40 User image christian 2011-12-29 14:42:41 PST
Comment on attachment 583959 [details] [diff] [review]
backout patch

Can we get the backout patch reviewed as well? It isn't a straight backout...
Comment 41 User image Olli Pettay [:smaug] 2011-12-29 14:54:07 PST
Comment on attachment 583959 [details] [diff] [review]
backout patch

I don't know who is on vacation.
Comment 42 User image Olli Pettay [:smaug] 2011-12-29 15:03:34 PST
Oh, hmm, the backout is for 9.0.x,
do we want to take it to Aurora/Beta too? For those backing out the original patch should be enough.
Comment 43 User image Olli Pettay [:smaug] 2011-12-29 15:05:17 PST
Or not quite. I'll upload a patch for Aurora/Beta backout
Comment 44 User image Olli Pettay [:smaug] 2011-12-29 15:12:30 PST
Created attachment 584865 [details] [diff] [review]
for aurora
Comment 45 User image Olli Pettay [:smaug] 2011-12-29 15:13:01 PST
Created attachment 584866 [details] [diff] [review]
for beta
Comment 46 User image christian 2011-12-29 15:35:30 PST
I think at the very least we want it for beta. We need to figure out what to do / how to kill the property for good (or if it is even worthwhile anymore, that much isn't clear).
Comment 47 User image Boris Zbarsky [:bz] (still a bit busy) 2011-12-29 20:02:54 PST
Comment on attachment 583959 [details] [diff] [review]
backout patch

r=me.

We should probably try to escalate this to the specs one way or another.... In practice, any UA that sniffs like Gecko or WebKit and does not have this property needs to include some WebKit bugs, so once WebKit fixes those bugs it may have to implement this property as well...
Comment 48 User image Eric Shepherd [:sheppy] 2011-12-30 10:22:46 PST
OK, so taintEnabled is back, I take it? As of which release? Is it back in 11 or did it make it back in time for 10?
Comment 49 User image Johnny Stenback (:jst, jst@mozilla.com) 2011-12-30 13:57:39 PST
Comment on attachment 583959 [details] [diff] [review]
backout patch

r=jst too, and sicking is on vacation IIRC.
Comment 50 User image Daniel Veditz [:dveditz] 2012-01-01 11:18:01 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> A simple codesearch query seems to suggest that this is a pretty popular UA
> sniffing tool unfortunately
> <http://www.google.com/codesearch#search/&q=navigator.taintEnabled&type=cs>.

Oddly, properly escaping the regexp wildcard '.' returns around ten times as many hits, instead of fewer (3800 vs. 440).

http://www.google.com/codesearch#search/&q=navigator\.taintEnabled

plain taintEnabled returns over 600K hits on codesearch. Use it now before Google shuts it down.
Comment 51 User image Asa Dotzler [:asa] 2012-01-03 14:56:22 PST
Comment on attachment 583959 [details] [diff] [review]
backout patch

un-breaking some of the web.
Comment 52 User image Alex Keybl [:akeybl] 2012-01-04 13:17:25 PST
I heard that the previous backout patch didn't transplant cleanly to beta.

smaug/bz - Could you prepare new patches for aurora/beta? They can be considered pre-approved and landed once ready. Thanks!
Comment 53 User image Olli Pettay [:smaug] 2012-01-04 13:20:44 PST
The "for beta" patch certainly does apply cleanly to beta.
I'll land it soon.
Comment 54 User image Olli Pettay [:smaug] 2012-01-04 13:22:32 PST
Do we need to create new interfaces for beta/aurora?
Comment 56 User image Olli Pettay [:smaug] 2012-01-04 14:08:06 PST
Should we back this out from trunk?
Comment 57 User image Olli Pettay [:smaug] 2012-01-04 15:18:29 PST
Oops, the patches didn't work on branches after all, and it is too late for me to fix.
(Fix is trivial). Backing out for now, and I'll land tomorrow fixed patches.
Comment 59 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-05 10:44:21 PST
I assume given the timestamp this did not make the cut for 10.0b3, will make the cut for 10.0b4?
Comment 60 User image Alex Keybl [:akeybl] 2012-01-06 15:30:50 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #59)
> I assume given the timestamp this did not make the cut for 10.0b3, will make
> the cut for 10.0b4?

The fix in Comment 58 will be available in 10.0b4.
Comment 61 User image Alex Keybl [:akeybl] 2012-01-31 20:19:56 PST
From my read of things, this is still present on FF12. What's the plan moving forward? Without outreach, I think we should back this out of m-c/Aurora 12.
Comment 62 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-10 20:50:12 PST
We should back this out of Aurora 12.

Not sure about m-c.
Comment 63 User image Alex Keybl [:akeybl] 2012-03-02 16:29:33 PST
(In reply to Olli Pettay [:smaug] from comment #56)
> Should we back this out from trunk?

Olli - can we perform this backout on both Aurora 12 and m-c? I don't think code cleanup like this is worth continuously backing out unless we want to perform outreach to the sites listed in comment#50 soon. Thanks in advance.
Comment 64 User image Johnny Stenback (:jst, jst@mozilla.com) 2012-03-13 18:32:42 PDT
Backed out of aurora and beta:

https://hg.mozilla.org/releases/mozilla-aurora/rev/f1135f99a0e4
https://hg.mozilla.org/releases/mozilla-beta/rev/49cca9941155

And I went ahead and backed this out of inbound as well, as lame as that is:

https://hg.mozilla.org/integration/mozilla-inbound/rev/db122c1048d7
Comment 65 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-03-13 22:39:44 PDT
One way forward might be to add a warning any time someone even checks if this function exists. I.e. if someone does

if (navigator.taintEnabled) { ...

or

if ("taintEnabled" in navigator) { ...

As well as add telemetry. Then leave it in that state until we see the telemetry dropping "low enough".

Not sure if it's worth the effort though.
Comment 66 User image Johnny Stenback (:jst, jst@mozilla.com) 2012-03-13 23:36:10 PDT
Yeah, agreed. We could add code in the resolve hook for navigator that would warn in the error console any time someone resolves taintEnabled, should not be hard to do at all.
Comment 67 User image :Ms2ger (⌚ UTC+1/+2) 2012-03-14 03:13:32 PDT
I would have been happy to see it die, but I'm not sure if this is a battle we should concentrate on. If we don't, though, we do need to get it into a spec. (HTML, I guess.)
Comment 68 User image Masatoshi Kimura [:emk] 2012-03-14 03:25:01 PDT
(In reply to Ms2ger from comment #67)
> I would have been happy to see it die, but I'm not sure if this is a battle
> we should concentrate on. If we don't, though, we do need to get it into a
> spec. (HTML, I guess.)
If that's spec'ed, other browsers will have to implement this empty function. Then, they may be misidentified as Firefox. Spec'ing this will not improve interoperability.
Comment 69 User image Ed Morley [:emorley] 2012-03-14 14:32:24 PDT
Backout merged:
https://hg.mozilla.org/mozilla-central/rev/db122c1048d7
Comment 70 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-03-19 15:18:04 PDT
[Triage Comment]
Thanks for backing out, no longer tracking for 12.
Comment 71 User image Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-10-15 05:11:16 PDT
(In reply to Jonas Sicking (:sicking) from comment #65)
> One way forward might be to add a warning any time someone even checks if
> this function exists. I.e. if someone does
> 
> if (navigator.taintEnabled) { ...
> 
> or
> 
> if ("taintEnabled" in navigator) { ...
> 
> As well as add telemetry. Then leave it in that state until we see the
> telemetry dropping "low enough".
> 
> Not sure if it's worth the effort though.

I would really like to see this possible so that we can make informed judgments about other features we might want to drop, such as .isSupported() etc.  If we're going to try removing old features that we hope not many pages use, we should have a reliable way of telling whether it's safe, not just wait to break pages and back out.
Comment 72 User image Andrew McCreight [:mccr8] 2012-10-15 06:13:48 PDT
It is really easy to hook things up to telemetry.  See bug 717711 for an example. You have to add something to TelemetryHistograms.h, then add a call to Telemetry::Accumulate somewhere. For this, I'm not sure if you'd want "total times we've ever seen a page with this", or just "have ever seen a page with this during the current telemetry session".
Comment 73 User image Philip Jägenstedt 2015-01-29 20:06:31 PST
We had a look at implementing navigator.taintEnabled() in Blink but reopened the spec bug instead:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=22555#c3

Please help us figure out what the spec should say.
Comment 74 User image j.j. 2015-02-01 09:15:49 PST
It was proposed to add a warning more than 3 years ago
(comment 15, comment 39, comment 71, etc.)
Shouldn't it be don now?
Comment 75 User image Boris Zbarsky [:bz] (still a bit busy) 2015-03-04 21:14:30 PST
Adding a warning naively won't help because people aren't calling the function, just checking that it exists....

We could make this a replaceable attribute instead, and warn in the getter, I guess.
Comment 76 User image Kohei Yoshino [:kohei] 2015-10-27 23:07:51 PDT Comment hidden (obsolete)
Comment 77 User image Philip Jägenstedt 2015-10-28 03:42:03 PDT
Is the removal actually in progress? If it is removed, please file a bug at https://whatwg.org/newbug so that we can remove it from the HTML spec as well.
Comment 78 User image Boris Zbarsky [:bz] (still a bit busy) 2015-10-28 06:46:49 PDT
I am not aware of any plans to remove it at this time, and I'm not sure why we're bothering with adding it to the compat doc...
Comment 79 User image Kohei Yoshino [:kohei] 2015-10-28 06:50:42 PDT
Removed the doc.

Note You need to log in before you can comment on or make changes to this bug.