142502 - wizard is broken for remote XUL

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Axel Hecht]

the wizard.xml constructor tries to get to a stringbundle, which fails for 
remote xul due to security constraints. I get a js error
"Permission denied to get property UnnamedClass.classes" and the widget is dead
after that. (See the sample on xulplanet.)
Now, the right fix would be (IMHO) to make stringbundle and applocale service
play nice. But this would require a security check in stringbundle.getBundle,
which, given the current performance constraints, doesn't look so good to me.
So I created a little patch, that at least fails gracefully and falls back to
english button labels.

Comment 1

[Axel Hecht]

Attachment: try{}catch around Components.classes, fall back to english

Here is the patch to wizard.xml. I'm really not sure if this is the right 
thing to do, or if fixing stringbundles is *so* right that we'd better do that.

Comment 2

[Jeff Patterson]

not sure if this is the same issue but I am also having trouble loading XUL apps
 Example: Accessing http://www.mozilla.org/docs/xul/xulnotes/template-primer.xul
produces Error: uncaught exception: Permission denied to call method
HTMLDocument.write
Even locally stored XUL refuses to load unless registered as chrome. This bug
really sucks for those of us trying to learn to use XUL for moz app development.
Is there a pref I can use to disable XUL security?

build:Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20021208

Comment 3

[[e-mail]]

Comment on attachment 82463 [details] [diff] [review]
try{}catch around Components.classes, fall back to english

Wouldn't it be easier to do everything using a DTD instead?

>+  	  this._bundle = stringBundleService.createBundle(bundleURL, localeService.GetApplicationLocale());
Nit: createBundle now only takes a URL (nobody fixed the JS callers, sigh...)

Comment 4

[Nickolay_Ponomarev]

Attachment: switch toolkit to DTD

This puts the button labels to a DTD file, but leaves default title in properties, meaning remote XUL will have to provide titles for all pages until bug 59701 is fixed.

I'm not sure I want to make an xpfe version, since it is already not in sync with toolkit's wizard.xml, and can be simply replaced with toolkit version in bug 282191.

Comment 5

[[e-mail]]

Comment on attachment 206681 [details] [diff] [review]
switch toolkit to DTD

>-          if (!label && this.onFirstPage)
>+          if (!label && !this._bundle)
>+            label = "";
>+          else if (!label && this.onFirstPage)
> #ifdef XP_MACOSX
>             label = this._bundle.GetStringFromName("default-first-title-mac");
> #else
>             label = this._bundle.formatStringFromName("default-first-title", [this.title], 1);
> #endif
>           else if (!label && this.onLastPage)
It seems to me the intent would be more obvious if you simply added && this._bundle to each of the two original ifs.

Note that I haven't tried this out in any way, so you need an additional review.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 206681 [details] [diff] [review]
switch toolkit to DTD

Locale files (esp. in browser/toolkit/mail) shouldn't be preprocessed. For toolkit, preprocess the wizard binding instead.

Comment 7

[Nickolay_Ponomarev]

Attachment: patch v2

Updated to comments

Comment 8

[[e-mail]]

Comment on attachment 207733 [details] [diff] [review]
patch v2

>+          else if (!label)
>+            label = "";
This is unnecessary as the only way !label can succeed is if the label was already empty.

>+<!ENTITY  button-cancel-win.label      "Cancel">
You decided against a common cancel entity?

Comment 9

[Nickolay_Ponomarev]

(In reply to comment #8)
> (From update of attachment 207733 [details] [diff] [review] [edit])
> >+          else if (!label)
> >+            label = "";
> This is unnecessary as the only way !label can succeed is if the label was
> already empty.
> 
I didn't want a call to setAttribute("label", null) happen, but since happens rarely and the same thing happens to description attribute, I can remove that. (Will do after mano's review.)

> >+<!ENTITY  button-cancel-win.label      "Cancel">
> You decided against a common cancel entity?
> 
Yes, I thought it was better to have consistently named entities than to save a few bytes here, especially since the file is not going to be preprocessed anymore.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 207733 [details] [diff] [review]
patch v2

r+a=mano if you do remove the |if (!label)| condition ;)

Sorry for latency.

Comment 11

[Nickolay_Ponomarev]

Attachment: patch v3

Thanks for the reviews and for the reminder.

Comment 12

[Asaf Romano (gone)]

Trunk:
Checking in toolkit/content/widgets/wizard.xml;
/cvsroot/mozilla/toolkit/content/widgets/wizard.xml,v  <--  wizard.xml
new revision: 1.28; previous revision: 1.27
done
RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/global/wizard.dtd,v
done
Checking in toolkit/locales/en-US/chrome/global/wizard.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/wizard.dtd,v  <--  wizard.dtd
initial revision: 1.1
done
Checking in toolkit/locales/en-US/chrome/global/wizard.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/wizard.properties,v  <--  wizard.properties
new revision: 1.6; previous revision: 1.5
done

Comment 13

[Asaf Romano (gone)]

Brnahc:
Checking in toolkit/locales/jar.mn;
/cvsroot/mozilla/toolkit/locales/jar.mn,v  <--  jar.mn
new revision: 1.25.2.7; previous revision: 1.25.2.6
done
Checking in toolkit/locales/en-US/chrome/global/wizard.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/wizard.dtd,v  <--  wizard.dtd
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in toolkit/locales/en-US/chrome/global/wizard.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/wizard.properties,v  <--  wizard.properties
new revision: 1.4.8.2; previous revision: 1.4.8.1
done
Checking in toolkit/content/widgets/wizard.xml;
/cvsroot/mozilla/toolkit/content/widgets/wizard.xml,v  <--  wizard.xml
new revision: 1.23.2.10; previous revision: 1.23.2.9
done

and on trunk, i forgot:
Checking in jar.mn;
/cvsroot/mozilla/toolkit/locales/jar.mn,v  <--  jar.mn
new revision: 1.30; previous revision: 1.29
done