Closed Bug 912322 Opened 11 years ago Closed 11 years ago

document.getAnonymous* should not be available to web content

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox-esr24 28+ verified

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(4 files)

We currently expose these via WebIDL, with a comment grumbling about how we need to expose them to avoid breaking in-content XBL. I'm not sure if the [Func=] infrastructure was available when that comment was written, but these days I'm pretty sure that we can just use IsChromeOrXBL, or potentially IsChromeOrXBLOrRemoteXUL. I'll write up a patch to try it out.
I had often wondered about the usefulness of these functions at the content level because most things you get back are usually inaccessible, its good to know I wasn't just over thinking things :-)
> I'm not sure if the [Func=] infrastructure was available when that comment was written It wasn't. Thank you for catching this; hiding these behind IsChromeOrXBL makes total sense!
IsChromeOrXBLOrRemoteXUL would be safer.
Remote-XUL-whitelisted pages will use XBL scopes (except for the tests, it is the reason test_popupanchor.xul failed). I think we should just fix the tests rather than adding yet another function only for our tests.
(In reply to Masatoshi Kimura [:emk] from comment #5) > Remote-XUL-whitelisted pages will use XBL scopes (except for the tests, it > is the reason test_popupanchor.xul failed). I think we should just fix the > tests rather than adding yet another function only for our tests. It's actually the other way around - remote XUL _doesn't_ use XBL scopes (though our tests do for the most part). This also means though that IsChromeOrXBL will return false for XBL running in remote XUL. With the XBL bit gone, we don't have any way of detecting the difference anymore. :-( Maybe we should actually make IsChromeOrXBL return true for remote XUL? jst's stated policy was that we care more about compat than security in remote-xul-whitelisted domains.
Flags: needinfo?(bzbarsky)
Yeah, looks like our own bindings (marquee, audio, video) are all kinds of broken for remote XUL (as evidenced by the reftest failures in comment 2, since reftests run without XBL scopes). I think we should make IsChromeOrXBL return true for remote XUL.
In fact, we already do the same trick in nsContentUtils::IsCallerXBL().
I could live with that, yes.
Flags: needinfo?(bzbarsky)
This brings us into alignment with nsContentUtils::IsCallerXBL(). We also take the opportunity to clean up some comments and invariants that changed with the removal of the XBL bit.
Attachment #799794 - Flags: review?(bzbarsky)
Attached patch Part 2 - Fix tests. v1 (deleted) — Splinter Review
The crashtest changes are untested (aside from the fact that they don't crash).
Attachment #799795 - Flags: review?(bzbarsky)
Attachment #799796 - Flags: review?(bzbarsky)
Comment on attachment 799794 [details] [diff] [review] Part 1 - Update semantics of IsChromeOrXBL to return true for remote XUL. v1 r=me
Attachment #799794 - Flags: review?(bzbarsky) → review+
(In reply to Bobby Holley (:bholley) from comment #6) > It's actually the other way around - remote XUL _doesn't_ use XBL scopes > (though our tests do for the most part). Oh, I somehow remembered it in reverse. Sorry for the confusion.
Comment on attachment 799795 [details] [diff] [review] Part 2 - Fix tests. v1 r=me
Attachment #799795 - Flags: review?(bzbarsky) → review+
Comment on attachment 799796 [details] [diff] [review] Part 3 - Stop making XBL methods available to the web. v1 Sweet. r=me
Attachment #799796 - Flags: review?(bzbarsky) → review+
(In reply to Bobby Holley (:bholley) from comment #13) > Created attachment 799796 [details] [diff] [review] > Part 3 - Stop making XBL methods available to the web. v1 This is headed in the direction I wanted to see happen way back with bug 817922, the binding names available as windows properties still bugs me. Cant wait to see how it comes together after everything though.
Well, green try push on a single desktop platform in comment 10. Remember back in bug 898939 comment 8 when it seemed easier to skip a crashtest on Android than fix the harness so you could set the pref? Might be better to fix the harness, and get rid of that skip and the other two fails-ifs. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b2730cc16df1 for https://tbpl.mozilla.org/php/getParsedLog.php?id=27412385&tree=Mozilla-Inbound etc.
(In reply to Phil Ringnalda (:philor) from comment #20) > Well, green try push on a single desktop platform in comment 10. > > Remember back in bug 898939 comment 8 when it seemed easier to skip a > crashtest on Android than fix the harness so you could set the pref? Might > be better to fix the harness, and get rid of that skip and the other two > fails-ifs. I am in awe of whatever combination of detective work, memory, or general omniscience went into this analysis. https://tbpl.mozilla.org/?tree=Try&rev=ca392209dbe5
This is what we probably should have done in bug 898939 comment 8.
Attachment #800272 - Flags: review?(jgriffin)
Attachment #800272 - Flags: review?(jgriffin) → review+
Looks like reftests have a special name on android :-( https://tbpl.mozilla.org/?tree=Try&rev=dac34559f4cf
Depends on: 914537
This doesn't meet ESR landing criteria, it can go out with the next major ESR version.
Flags: needinfo?(lsblakk)
Flags: needinfo?(lsblakk)
Comment on attachment 799796 [details] [diff] [review] Part 3 - Stop making XBL methods available to the web. v1 [Approval Request Comment] User impact if declined: Recent XBL improvements hamstrung. Fix Landed on Version: 26 Risk to taking this patch (and alternatives if risky): Removes a function that's been available to web content, but we've already shipped this on release without problems. String or UUID changes made by this patch: None
Attachment #799796 - Flags: approval-mozilla-esr24?
Attachment #799796 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
This doesn't play nicely with esr24. Mind taking a look, Bobby? :)
Flags: needinfo?(bobbyholley)
Confirmed issue on 24.3esr Verified fixed on 24.3esrpre built on 2014-03-12.
I just want to add a comment that this change has somewhat broken IBM's web mail, called iNotes: http://planetlotus.org/profiles/bill-malchisky_121034 Now, I'm not going to defend IBM, I have been forced to use Lotus for over 15 years now, I could write a huge diatribe about everything that I hate about it. Nevertheless, I somewhat take exception to the conclusion that justifies the risk: "but we've already shipped this on release without problems" You may not hear about problems because users simply switch to IE or Chrome, find that they don't have the problem, discard Firefox, and move on. I've basically done that (a year ago) at work. I think Chrome's memory consumption is high so I'm again trying to use it at work, but I find it is basically unusable with IBM's products, it has plenty of issues that Chrome simply doesn't have. I'll try to find the root cause for those other issues and add comments to the appropriate regressions, if I can find them. This is sort of a big regression with no warning -- why haven't the other browsers removed these methods? This means that Firefox won't work in some scenarios where others do -- and I think Mozilla should do everything it can to PREVENT that from happening. When my browser doesn't allow me to get work done, I get **** off, and I try a different one. I think other users do the same. IBM has released a fix for this, but as an end user (and not an IT person at my company) I need to wait for our IT team to implement the fix, and with ~200k employees most times that isn't trivial. I mean no offense, I just wish you would slowly deprecate when you make changes like this (removing something that was there for a long time) -- maybe hide the methods with a config variable, maybe provide a warning if the method is accessed, then if users have issues they can un-hide them? Lotus iNotes: Chrome works, IE works, Firefox doesn't.
> why haven't the other browsers removed these methods? These methods never existed in the other browsers in the first place. So if removing them changed something in iNotes, that means iNotes was running different code in different browsers and relying on non-standard functions in Firefox for some reason. We usually do deprecate things slowly, and maybe we should have done that here. We made the (clearly mistaken) assumption that since no other browser supported them usage in the wild was unlikely; the only usage we had seen in practice was as part of security exploits (which is why we wanted to remove them somewhat expeditiously).
Thank you for the response Boris. This isn't the first time Mozilla has broken iNotes. (One could argue iNotes is just inherently broken to begin with, and I would not argue with them at all!) Remote XUL was suddenly disabled at one point in the past, but luckily there was an extension where one could whitelist certain sites who want to use remote XUL. iNotes was one of them. But even with a fix existing, when something one day suddenly just *breaks*, a user just switches to another installed browser, you know? I usually have three installed at all times. Sadly. As of a few days ago iNotes is broken further in FF due to the sudden removal of SSLv3. Won't load at all. I found the extension as a work around. Most (99%) users won't bother searching. Chrome and IE don't have this issue (yet -- I understand it is coming). I don't know why iNotes is using these methods or what they're doing with them. Their js is completely obfuscated and I can't read it. I wonder if Greasemonkey could be used to inject the method again, thus circumventing this issue?
Oh, if they're using remote XUL, I guess it's not that surprising that they were using document.getAnonymous*. But it also means they are using something that's not only deprecated but has been removed for quite a long time... which suggests to me even if we were to try deprecating getAnonymous* it would not affect iNotes one whit. :( I do sympathize with the "things are suddenly broken" pain. There was a tradeoff here between the security issues involved in keeping these methods exposed and the impact of having these non-standard functions on the web and the potential for breakage. I do think we made the right decision on that tradeoff, but I realize that doesn't make your life any better. :( You could certainly use an extension to inject these methods. I don't know whether Greasemonkey in particular can do it; it would need to inject methods running with the system principal. But it it can, then yes, that would work.
Most likely iNotes keep breaking because they try to use remote XUL in Firefox. While using standard methods in IE and Chrome. It's likely to keep breaking since remote XUL is and never was particularly stable. If they just changed to use the same code for Firefox as they do for IE and Chrome it would likely work much better. And to be clear, remote XUL was very publicly deprecated a long time ago. You even have to run an addon in order to enable it. Sadly there's not much we can do about that on our side. And this bug is definitely not the right forum to discuss it.
@Jonas, thanks for your reply. I'm done after this but, I clearly said that I in the past used an add on to enable remote XUL. I think iNotes stopped using remote XUL a couple of years ago, and our IT department did make the necessary updates to get the non-XUL implementation. Maybe the calls to document.getAnonymous* are artifacts from the old XUL usage, if, like Boris said, there is an obvious connection between remote XUL and these getAnonymous* methods. So remote XUL isn't really a part of this conversation, it is basically a part of the "these things were removed and suddenly broke what I'm doing" discussion, which is how I arrived at this bug. When you say something was "very publicly deprecated" that doesn't mean the same thing to a huge majority of your users who have *no clue* what remote XUL *is*. All they know is that their web email doesn't work. Period. My company of 200k employees uses iNotes as their webmail. I don't like it, but that's what I'm given. This bug breaks it, and I linked earlier to a Lotus forum post talking about it. @Boris, thank you very kindly for your courteous reply, maybe with "unsafeWindow" I can inject the methods via Greasemonkey. I might try it but for now I'm going to use Chrome because that is the easiest workaround for me.
The document.getAnonymous* functions are essentially only useful if you use remote XUL (I consider XBL as part of XUL at this point). So if iNotes is still using document.getAnonymous*, then it's still using remote XUL.
The fallout from remote XUL removal and iNotes is in bug 633113. I don't seem to have the remote XUL manager installed so -- I don't know how iNotes mostly works unless I try to create a new mail with an email already open, then I get this bug. This is all sort of moot since SSLv3 means you can't get to iNotes at all anymore without another add-on.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: