623482 - Present error message when someone tries to use remote XUL

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Currently if someone tries to create a XUL element we simply fail the action and throw an error. The result is often a blank or nonsensical page.

If possible, we should use our standard notification mechanisms to display an error message saying that the page is trying to use a outdated or now-disabled feature.

I believe our current notification method of choice is the door hangers, so that's probably our best option here too.

Since the message is part of the UI it should ideally be localized. But given that we're way past string freeze that might not be possible. Alternatively we can just always display an english error message. I'll leave that decision up to people that knows our l10n policies better.

Note that it's expected that very few people will see this given that very few sites out there use XUL.

Also note that I fairly strongly think that we should *not* provide links or instructions for how to enable XUL for the given site given that we consider XUL a attack vector. Hence the best we can do in the message is to tell people to contact their web master to tell them to upgrade their site.


I personally don't think we should do this if it introduces too much risk or other complications, hence I don't think it's a blocker. But we should look into it.

Comment 1

[Mounir Lamouri (:mounir)]

Pike, what is the best way to deal with the l10n issue here?

Comment 2

[Axel Hecht [:Pike]]

My take would be, either it's a blocker, than we should do it with l10n, or it's not a blocker. Then we shouldn't bother at all.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

And my take is that this isn't a blocker, so I think we're looking at punting or outright WONTFIXing.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

So, this is showing an error message when the user tries to load a XUL page which can't be loaded because of security reasons. The only issue of this implementation is that the error message will be always shown on the current tab. I should find a way to know which tab tried to load the XUL page but I wonder how to do that.

However, I think how remote xul is currently handled is weird : nothing happens; which can make this error message very inappropriate. For example, go to mozilla.org then type "oldworld.fr/mozilla/window.xul" and you will see the popup (with the patch applied) but mozilla.org is still loaded.
Jonas, were you expecting this bug to fix this issue (load a blank page for example)?

I'm wondering if we couldn't use netError.xml when trying to access remote XUL instead of showing a simple popup.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I'm wondering if we couldn't use netError.xml when trying to access remote XUL
> instead of showing a simple popup.

You could change the code you're changing there to return a new error code of some sort, then have nsDSURIContentListener::DoContent cancel |request| with that error code.  Then you should end up in the error page codepath for that error code, and it's just a matter of adding handling to it.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch with only one new string

So, this patch now consider two situations:
1. You are trying to access a XUL document and it's not allowed
   -> an error page is shown
2. You are trying to access a (XML) page which tries to create XUL elements
   -> a popup informs you that the page might not work as expected

For the moment, I'm using the error page used for "unsafe content type" which seems appropriate. IMO, it sounds to be a good compromise to prevent adding new strings to translate so late.
However, I can't use an existing string for the popup. For the moment, the string is in english only. By the way, Jonas, could you give me a text? Jonas thinks this popup should be rare enough to be kept in english. Pike, is adding one string a big deal? (I say one assuming that we can keep the unsafe content type for the error page)

Comment 7

[Axel Hecht [:Pike]]

At this point, every string is a peppermint. So, yes, big deal.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For what it's worth, I'd rather display a blank page and a XUL-is-disabled-specific error message in a door hanger, than a generic "unsafe content type" neterror page.

While XUL being too unsafe is the reason we're disabling it, I don't think that's how authors or users think about it, thus I don't think using that description is helpful to them.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So unless we can get a always-english XUL-specific error, i'd rather not use a error-page.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> So unless we can get a always-english XUL-specific error,

You mean unless we decide to do that? Seems do-able and better than blank page. I'm not saying we must, just ranking less-worse.

> i'd rather not use a error-page.

What's the alternative?

/be

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #10)
> (In reply to comment #9)
> > So unless we can get a always-english XUL-specific error,
> 
> You mean unless we decide to do that? Seems do-able and better than blank page.
> I'm not saying we must, just ranking less-worse.

Note that I'm saying "blank page with error message in door-hanger".

What I mean is "So unles we can get an always-english XUL-specific error page, without having to too big and unsafe modifications to our error-page implementing code, I don't think we should use a error page".

> > i'd rather not use a error-page.
> 
> What's the alternative?

blank page with error message in door-hanger.

What I'm saying is that I think it's more important that we have an useful error message, than that that error message is presented in any particular way.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > So unless we can get a always-english XUL-specific error,
> > 
> > You mean unless we decide to do that? Seems do-able and better than blank page.
> > I'm not saying we must, just ranking less-worse.
> 
> Note that I'm saying "blank page with error message in door-hanger".
> 
> What I mean is "So unles we can get an always-english XUL-specific error page,
> without having to too big and unsafe modifications to our error-page
> implementing code, I don't think we should use a error page".

It's easily doable.

Can you give me the strings you would like to see? (for the error page and the popup message)

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For the door-hanger:

"This page uses a deprecated technology which is no longer available in Firefox. This can result in the page being displayed wrong or not at all."

For the error page (if we use one), something like:

"This page uses a deprecated technology which is no longer available in Firefox. Because of this we are unable to display the page."

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to comment #13)
> For the error page (if we use one), something like:
> 
> "This page uses a deprecated technology which is no longer available in
> Firefox. Because of this we are unable to display the page."

Error pages have a title, a short description and a long description.
Usually, the long description seems to explain what to do.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, the whole crux of the matter here is that we can't tell the user what to do, as there is no known safe action. We don't want them going to evil.com and have firefox merrily show a message how to enable XUL just so that they get exploited.

I'll ask in the other bug for further ideas.

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #13)
> For the door-hanger:
> 
> "This page uses a deprecated technology which is no longer available in
> Firefox. This can result in the page being displayed wrong or not at all."

s/wrong/incorrectly,/ (with comma)

Re: comment 15 -- I don't think we need to tell them how to find the add-on with a link or anything. We could talk about using an older version of Firefox.

/be

Comment 17

[bugzilla-mozilla]

First and foremost, thank you for considering adding an error message. I represent one of those few remote XUL developers and will continue to use it until something better comes along.

> "This page uses a deprecated technology which is no longer available in
> Firefox. This can result in the page being displayed wrong or not at all."

I don't think "which is no longer available in Firefox" is right, because it is if you want it to. I think it will just be confusing if we have to explain that the error message isn't telling the truth when the technology is still in there and you just have to whitelist your domain.

Instead I propose something along the lines of: "This page uses an unsupported technology that's no longer enabled by default in Firefox."

It would be simply great if we could have a notice, link, anything that will point the users that DO want to enable it in the right direction rather than having them sitting there scratching their heads. But from what I gather that's not how you see it.

Comment 18

[Unknown W. Brackets]

I think "enabled by default" moves away from what Jonas Sicking is trying to communicate here.  Perhaps:

"This page uses a deprecated technology which requires additional software to use in Firefox. This may result in the page being displayed incorrectly, or not at all."

Completely true and not misleading, and would lead most clients to send an email or give a ring, I think, which should lead anyone to the desired result.

That said, no translation does suck as a rule of thumb.  If it went English-only, would it get changed in a future release (4.x or 4.0.x), or just left that way?

-[Unknown]

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> Well, the whole crux of the matter here is that we can't tell the user what to
> do, as there is no known safe action. We don't want them going to evil.com and
> have firefox merrily show a message how to enable XUL just so that they get
> exploited.

What to do doesn't mean "how to enable it" but it could just be "inform the webmaster and your IT department" for example.

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Patch

So, this is using a specific error page with english-only text.
The strings might not be final but the entire logic should not change now, I guess.

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Screenshot of the error page

Comment 22

[Mounir Lamouri (:mounir)]

Attachment: Screenshot of the popup

Comment 23

[Dão Gottwald [:dao]]

The popup won't allow copy/paste, so please use the error page. The error page seems to make more sense too, since there's no other meaningful content to display.

Comment 24

[Mounir Lamouri (:mounir)]

(In reply to comment #23)
> The popup won't allow copy/paste, so please use the error page. The error page
> seems to make more sense too, since there's no other meaningful content to
> display.

The popup and the error page are not used for the same reasons with this patch. The popup is shown when there is a XUL element trying to be added to the document and the error page happens when you try to load a XUL document.

Are you asking to always have an error page in both situations?

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> The popup is shown when there is a XUL element trying to be added to the
> document and the error page happens when you try to load a XUL document.

I didn't realize this. Looking at the screenshots again, I still would prefer the error page in both situations, given that the syntax error page doesn't seem overly useful.

Comment 26

[Jorge Villalobos [:jorgev] (he/him)]

> "This page uses a deprecated technology which is no longer available in
> Firefox. This can result in the page being displayed wrong or not at all."

I think the word "deprecated" is too technical. "outdated" or "unsupported" would be clearer IMO.

Comment 27

[Brendan Eich [:brendan]]

+1 on comment 26. Deprecation means speaking against, prior to obsoleting. We've obsoleted in the absence of an add-on and add-ons can do almost anything, so we've obsoleted.

But I would not use the canonical companion-word "obsolete" here -- "unsupported" seems best.

Usage nit: "that" instead of "which" for defining clauses. The " is no longer available in Firefox" is the defining clause here, because "unsupported" does not imply "not built in".

/be

Comment 28

[Brendan Eich [:brendan]]

+1 on comment 25 too -- popups suck, error pages rule for this kind of all-or-nothing-in-reality failure. I don't believe anyone is doing fallback from remote XUL to HTML.

/be

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please note:

1. The door-hanger almost only appear in pages that use the DOM to create XUL. This is rare. People that use full XUL documents will not see the door hanger. It probably happens, not in situations when people try to do fallback, but rather in situations when they want a little bit of XUL in a mostly HTML page.

2. If we were display the error page even in the above situation, we both will have to add a lot more complexity, and risk completely breaking pages that would at least partially work

3. This is affecting very few people. We don't need a perfect solution. We have much more important things to do.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 503115 [details] [diff] [review]
Patch

Only send out the forbidden-remote-xul once per document (i.e. keep a flag on nsIDocument indicating if it's been sent out or not).

Probably safer to send it out asynchronously too, just in case addons do something crazy with it.

r=me with those changes.

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #29)
> Please note:
> 
> 1. The door-hanger almost only appear in pages that use the DOM to create XUL.
> This is rare.

Then don't cover it, since it adds cruft to front-end code? (Which needs separate review, btw.)

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure what you mean by "don't cover it"?

Happy to have someone else review the front-end parts.

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> Not sure what you mean by "don't cover it"?

Removing it from the patch. Seems like we don't really need it if it's an edge case.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This whole bug is an edge case. As previously stated, I'm fine with closing this bug as WONTFIX.

Comment 35

[Dão Gottwald [:dao]]

Well, things aren't black or white. Remote XUL is an edge case, but remote XUL in HTML is an edge case of an edge case, right?

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Basically what I'm saying is, if you think we should report less, it's not me that you need to convince, but rather Brendan and the people over in bug 546857.

Comment 37

[Dão Gottwald [:dao]]

I actually agree with what people said, the empty page seems pretty catastrophic for intranets where people can freely update Firefox, but the allegedly less common XUL fragments don't seem to suffer from this as badly (there's the XML error page).

Comment 38

[Boris Zbarsky [:bzbarsky]]

> popups suck, error pages rule for this kind of all-or-nothing-in-reality
> failure

DOM creation of XUL is not all-or-nothing.

Or put another way, just because an ad on a page creates a XUL element doesn't mean we should replace the entire page with the XUL error page.  At least I wouldn't sign off on that sort of thing.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I feel very strongly that we should not mess around with replacing a existing page just because a XUL element is created. That leaves us with these two choices:

A) Don't attempt any "error handling" for XUL elements being created. Just have warnings when the XUL mime-type is used.

B) Do what the current patch is doing.


Anything else is too complex and/or too risky. Both for what we're trying to do (give a comforting "we feel your pain, you're not alone" message) and for when we're trying to do it (this late in the release cycle. This part is my fault).

So in short. We should either take Mounir's patch (modulo any review fixes), or a strict subset of it which doesn't do the door-hanger warning.

Comment 40

[bugzilla-mozilla]

For what it's worth, having been messing around with remote XUL apps longer than most people the only situation where I've seen XUL and HTML mixed is in tutorials that try to show the difference. It's really all or nothing. IBM iNotes, arguably the application with most users, only deliver remote XUL in separated frames, never inline with HTML from what I remember.

Really, I don't think just skipping the error message all together for DOM creation would hurt anyone. If it would just throw an error instead it would actually be useful for catching whether or not the domain was whitelisted.

(In reply to comment #26)
> > "This page uses a deprecated technology which is no longer available in
> > Firefox. This can result in the page being displayed wrong or not at all."
> 
> I think the word "deprecated" is too technical. "outdated" or "unsupported"
> would be clearer IMO.

Very much agree on this one. Can we agree on "Unsupported"? In my biased opinion calling remote XUL outdated isn't, from a technical standpoint, quite right either.

Comment 41

[Brendan Eich [:brendan]]

(In reply to comment #38)
> > popups suck, error pages rule for this kind of all-or-nothing-in-reality
> > failure
> 
> DOM creation of XUL is not all-or-nothing.

Of course, and we've been over this in this bug. But the reality of remote XUL is all or nothing, AFAICT. Counterexamples (non-test, real-world) welcome.

/be

Comment 42

[Brendan Eich [:brendan]]

Re: comment 39, by my own argument, sicking's option (A) should be tolerable.

Re: comment 40: agree on "unsupported", "outdated" is wrong since HTML+CSS < XUL in critical ways, even if it's >= in others.

/be

Comment 43

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Show an error page when trying to load a remote XUL page

This patch only includes the error page code with the string change requested in above comments.

The patch got r+ from Jonas so it's ready to be landed. Any string change request before I do that?

Comment 44

[Mounir Lamouri (:mounir)]

Comment on attachment 503516 [details] [diff] [review]
Part 1 - Show an error page when trying to load a remote XUL page

Hmmm, not really ready to go. It needs an approval :)

Comment 45

[Brendan Eich [:brendan]]

Should that hardcoded "Firefox" be navigator.product or better instead?

/be

Comment 46

[Mounir Lamouri (:mounir)]

Attachment: Show an error page when trying to load a remote XUL page

This is now using the brand short name instead of the hardcoded 'Firefox'.

Comment 47

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

The inter diff adding the usage of the brand short name.

Comment 48

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Show a popup when the page tries to create a XUL element

So, the event is now sent async but it's not sent only once like Jonas asked for. Instead of that, the observer ignores the event if the popup has already be shown.

IMO, I wonder why we wouldn't take this patch given that the code is here and should be safe. The only reason I see to not take this patch now is if we *never* show any kind of message in that situation.

Comment 49

[Robert Kaiser]

Why do we need a hardcoded string here and can't have a proper localized string for this?

Comment 50

[Brendan Eich [:brendan]]

(In reply to comment #49)
> Why do we need a hardcoded string here and can't have a proper localized string
> for this?

Please read the bug. We can, but not after l10n freeze. This is better than a blank page, even though it's English only for now.

/be

Comment 51

[Robert Kaiser]

Sorry, I hadn't noticed that we are in a 100% string freeze already.
Though in the past, we had situations where we used a technique of "use localized string if available and fall back to hardcoded" in some places like that, not sure if that's applicable, Axel would know, I guess.

Comment 52

[Dão Gottwald [:dao]]

(In reply to comment #48)
> IMO, I wonder why we wouldn't take this patch given that the code is here and
> should be safe.

Because browser.js is already bloated enough and somebody needs to maintain it. I don't think we should take the code just because it's there.

Comment 53

[Axel Hecht [:Pike]]

FWIW, I'm not in favor of doing this at all, beltzner wasn't in favor in comment 3 either.

Comment 54

[Brendan Eich [:brendan]]

A blank page for something we supported for over a decade is poor behavior a bug that we should fix when we can. If we don't fix it now, then we will fix it soon enough in the new quarterly release world -- and it's not valid to bootstrap a perpetual WONTFIX off of a decision not to fix late in Firefox 4.

There are lots of polish bugs, and more serious UX bugs, still to fix. This one is a fix nearly in hand and it has support from long-time Mozillans. I don't see why we wouldn't take it if it's ready, unless we WONTFIX, but WONTFIX was not sicking's call, it's not mine, and it does not match our standards for such bugs.

/be

Comment 55

[Brendan Eich [:brendan]]

(In reply to comment #54)
> WONTFIX was not sicking's call,

I know about comment 34, but in an email thread sicking ack'ed the bug. I'll let him record that here if necessary.

> it's not mine,

This I'm sure about.

/be

Comment 56

[Dão Gottwald [:dao]]

Comment on attachment 503537 [details] [diff] [review]
Part 2 - Show a popup when the page tries to create a XUL element

As far as I can tell, nobody really asked for this. We should be fine with part 1.

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I would have been fine with WONTFIXing this. But, engineering resources and timing aside, I would also be fine with taking the "part 1" patch.

I agree with Dao that if no-one is asking for the second error message then we shouldn't implement it. Despite having code in hand.

Comment 58

[Brendan Eich [:brendan]]

Sicking: noted, doesn't line up with mail thread. Also not the right thing for the users we're breaking; I guess that's something we'll have to disagree on.

Part 2 (the compound document case) is not needed, it seems everyone agrees.

/be

Comment 59

[Robert Kaiser]

IMHO, we still should take a proper L10n as it looks like we are still taking L10n changes e.g. for Sync, so adding this one string (with %S-replacement instead of plainly appending the brand name) should not really be worse. Of course, Axel has the last call on L10n, but that's what I'm seeing when looking at what's going on globally in the project.

Comment 60

[Axel Hecht [:Pike]]

"We're adding strings to ABC" isn't the argument that's going to make us get to RC by the end of the month. Each and every string is a problem at this point.

Comment 61

[Mounir Lamouri (:mounir)]

(In reply to comment #59)
> IMHO, we still should take a proper L10n as it looks like we are still taking
> L10n changes e.g. for Sync, so adding this one string (with %S-replacement
> instead of plainly appending the brand name) should not really be worse. Of
> course, Axel has the last call on L10n, but that's what I'm seeing when looking
> at what's going on globally in the project.

There are actually 3 strings (see the screenshot). Though, I would be really happy to update this patch and make the strings localizable but I think they are technical so I it might add a non-trivial work for the localization teams and as said Pike, it's probably not the best moment to add strings, isn't it?

Comment 62

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/b023a8c82f97

Comment 63

[Robert Kaiser]

(In reply to comment #61)
> There are actually 3 strings (see the screenshot). Though, I would be really
> happy to update this patch and make the strings localizable but I think they
> are technical so I it might add a non-trivial work for the localization teams
> and as said Pike, it's probably not the best moment to add strings, isn't it?

OK, please file a followup to make them correctly localizable in post-FF4 trunk though.

Comment 64

[Axel Hecht [:Pike]]

I personally hope that we'll triage if we can actually remove that code post FF4 again. One would hope that legacy xul on the web will go away once it's unsupported.

Comment 65

[Mounir Lamouri (:mounir)]

(In reply to comment #63)
> OK, please file a followup to make them correctly localizable in post-FF4 trunk
> though.

bug 625760

Comment 66

[Brendan Eich [:brendan]]

(In reply to comment #64)
> I personally hope that we'll triage if we can actually remove that code post
> FF4 again. One would hope that legacy xul on the web will go away once it's
> unsupported.

This is not realistic. Many remote XUL users won't upgrade right away, and we don't have good communication channels or stronger means to impose updates. There is no reason to rush to rip out this bug fix.

/be

Comment 67

[bugzilla-mozilla]

As far as I can tell from the source code the error message is stills saying that "the technology is no longer available in (Firefox)". Can't we drop this part? Because as stated it's both wrong and will just add to the confusion when we have to tell our users that the error message is lying and their application works just fine (in fact even faster in 4.0) as soon as they add their domain to the whitelist.

Comment 68

[Mounir Lamouri (:mounir)]

I've just pushed a small change. Now the message says that the feature is "no longer available by default".

http://hg.mozilla.org/mozilla-central/rev/542d3da64ac2