Closed
Bug 846251
Opened 12 years ago
Closed 11 years ago
XML parsing error in blocklist.xul
Categories
(Firefox Graveyard :: Web Apps, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 27
People
(Reporter: adora, Assigned: marco)
References
Details
(Keywords: regression, Whiteboard: [apps watch list] DesktopWebRT2)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Pike
:
feedback-
|
Details | Diff | Splinter Review |
Periodically, I get the following error message while running apps on Firefox Nightly on desktop:
XML Parsing Error: undefined entity
Location: chrome://mozapps/content/extensions/blocklist.xul
Line Number 18, Column 1:<dialog windowtype="Addons:Blocklist" title="&blocklist.title;" align="stretch"
Screenshot: http://adora.io/screens/blocklist_error_16DF6183.png
Right now I'm seeing it with an app in the review queue called DatKey, but there have been others. https://marketplace.firefox.com/reviewers/apps/review/datkey
Reporter | ||
Updated•12 years ago
|
Whiteboard: [apps watch list]
Comment 1•12 years ago
|
||
Probably a regression with the blocklist integration with desktop apps.
Sadly, the desktop implementation has collected a bunch of cruft as a result of advancing b2g work. And we have little resources allocated to it, so I don't have an ETA of when something like this would be fixed.
Keywords: regression
Reporter | ||
Comment 2•12 years ago
|
||
As long as the error doesn't mean I'm getting teh hax0rd, then this isn't a priority for me now.
Comment 3•11 years ago
|
||
Lisa please check to see if this bug is still present.
Assignee: nobody → lbrewster
Reporter | ||
Comment 4•11 years ago
|
||
Yep, still seen in Nightly 2013-06-05.
Reporter | ||
Comment 5•11 years ago
|
||
Yep, still seen in Nightly 2013-06-05.
Assignee: lbrewster → nobody
Assignee | ||
Comment 6•11 years ago
|
||
Could I have access to that application? Or, do you have another test case publicly accessible?
Flags: needinfo?(adora)
Reporter | ||
Comment 7•11 years ago
|
||
It happens randomly while using many desktop apps. I don't have STR, best advice I can give is to try a few apps from Marketplace and see if it comes up.
Flags: needinfo?(adora)
Updated•11 years ago
|
Priority: -- → P1
Assignee | ||
Comment 8•11 years ago
|
||
Same problem as bug 851217, I guess we can simply modify the strings to avoid using brand.dtd.
The guilty strings are:
"&brandShortName; has determined that the following add-ons are known to cause stability or security problems:"
"Restart &brandShortName;"
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #786736 -
Flags: review?(mark.finkle)
Comment 10•11 years ago
|
||
We might be able to simply avoid using brand-specific strings in some cases (or switch to strings generated dynamically from .properties files), but in this case the dialog in question shouldn't be showing up in the first place, since addons aren't enabled in the runtime.
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Myk Melez [:myk] [@mykmelez] from comment #10)
> We might be able to simply avoid using brand-specific strings in some cases
> (or switch to strings generated dynamically from .properties files), but in
> this case the dialog in question shouldn't be showing up in the first place,
> since addons aren't enabled in the runtime.
We're using the blocklist also for plugins.
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #11)
> We're using the blocklist also for plugins.
(That in our case is only Flash)
Comment 13•11 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #11)
> We're using the blocklist also for plugins.
Ah, sorry, I missed that in this case the word "add-ons" is being used in its more general sense rather than as a synonym for "extensions"!
So, in some cases, where Firefox uses its own name, we'll want to use the app's name (like we do in the titlebar). And in other cases we might want to drop the name. But in this case, I think we do want to identify Firefox as the agent of the blockage, so the user understands who/what is responsible for blocking the plugin.
Assignee | ||
Comment 14•11 years ago
|
||
Comment on attachment 786736 [details] [diff] [review]
fix_blocklistxul_webapprt
I see what you mean, some users could be confused and think that "Restart" means "Reboot your computer".
Attachment #786736 -
Attachment is obsolete: true
Attachment #786736 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 15•11 years ago
|
||
With this patch the blocklist dialog will use the webapp name, in the webapp runtime, or the branding name.
As I was there, I've also removed a hack introduced to avoid creating new strings during a string freeze.
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #787163 -
Attachment is obsolete: true
Attachment #787183 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 17•11 years ago
|
||
Forgot to qref...
Attachment #787183 -
Attachment is obsolete: true
Attachment #787183 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•11 years ago
|
Attachment #787184 -
Flags: review?(mark.finkle)
Comment 18•11 years ago
|
||
Comment on attachment 787184 [details] [diff] [review]
Patch
The webrt changes look OK to me, but the toolkit changes are out of my league.
Passing to Gavin to find a reviewer.
Attachment #787184 -
Flags: review?(mark.finkle)
Attachment #787184 -
Flags: review?(gavin.sharp)
Attachment #787184 -
Flags: review+
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 787184 [details] [diff] [review]
Patch
I'm waiting for Mossop ideas about how to handle this in general (he wasn't happy with the Services.appinfo.ID check).
Attachment #787184 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 20•11 years ago
|
||
We've thought that it's better if the users identify Firefox as the blocking agent.
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #787184 -
Attachment is obsolete: true
Attachment #791572 -
Flags: feedback?(l10n)
Comment 22•11 years ago
|
||
Note: Axel will be AFK for most of the next 10 days, and my knowledge of this area is far too limited to give meaningful feedback (for example I can't understand the reason behind the change in JarMaker.py).
I'll leave the f? open for Axel, just wanted to let you know in case the patch is urgent.
Comment 23•11 years ago
|
||
Comment on attachment 791572 [details] [diff] [review]
Patch
Review of attachment 791572 [details] [diff] [review]:
-----------------------------------------------------------------
I don't think this is the right way of doing. My problem is that I also don't exactly know what we're doing.
Is this brand.dtd consistently not being part of the right package? How is that package packaged? Can we do something like
https://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#152
but with a different xpi-name?
Attachment #791572 -
Flags: feedback?(l10n) → feedback-
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #23)
> I don't think this is the right way of doing. My problem is that I also
> don't exactly know what we're doing.
>
> Is this brand.dtd consistently not being part of the right package? How is
> that package packaged? Can we do something like
> https://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.
> in#152
> but with a different xpi-name?
The webapp runtime and Firefox have two separate packages. The problem is that we want the Firefox's brand.dtd and brand.properties to be packaged also for the webapp runtime.
The webapp runtime package is built using these two jar manifests: http://mxr.mozilla.org/mozilla-central/source/webapprt/jar.mn and http://mxr.mozilla.org/mozilla-central/source/webapprt/locales/jar.mn
So, I wanted to include the brand.dtd and brand.properties in the webapp runtime package, but the localized versions of those files can be in an absolute directory outside the Mozilla source code. This is why I had to modify JarMaker.py.
Comment 25•11 years ago
|
||
Seems the webapprt magic includes at least the http://mxr.mozilla.org/mozilla-central/source/webapprt/defs.mk with
DIST_SUBDIR=webapprt
I'm pretty sure we don't need or should modify jarmaker, but build browser branding with the right flags for webapprt. I.e., do that twice.
Gregory, can you guide us here, I'm traveling and don't have much time for trial and error (which is what I'd do ;-) )
Updated•11 years ago
|
Flags: needinfo?(gps)
Comment 26•11 years ago
|
||
I don't have an answer on the top of my head. One of the two Mike's should hopefully have insight.
Flags: needinfo?(mshal)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(gps)
Comment 27•11 years ago
|
||
I'm not too thrilled by the webapp runtime getting the firefox branding. In fact, I wanted to move it from app to platform, making it part of xulrunner, where such branding is not available.
Can't we get the webapp runtime to create a brand.dtd (virtually in memory or physically on disk) for each installed app instead?
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 28•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #27)
> I'm not too thrilled by the webapp runtime getting the firefox branding. In
> fact, I wanted to move it from app to platform, making it part of xulrunner,
> where such branding is not available.
>
> Can't we get the webapp runtime to create a brand.dtd (virtually in memory
> or physically on disk) for each installed app instead?
We need to use the Firefox branding in the blocklist dialog, because we want the user to identify that is Firefox that is blocking a plugin.
If we don't want to package the Firefox branding in the webapp runtime package, we could (I don't know yet how) read the Firefox branding file from the Firefox package when it is requested. This solution would probably be less easy.
Note that we can't store the branding on disk during installation, because the webapp could be executed by different versions of Firefox (Aurora, Nightly, that have their own branding). e.g. it could be installed through Nightly and later executed through Aurora, we'll want the Aurora branding in this case.
Comment 29•11 years ago
|
||
I was implying to use the app name for the app-specific branding.
That being said, I also don't see user identifying Firefox as blocking the plugin a good thing, because they'll get that prompt from Firefox itself saying it's Firefox (obviously), and then from every webapp, saying it's Firefox, at which point they'd likely say "Can't Firefox just get its act together and stop annoying me about that plugin already?".
Comment 30•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #29)
> That being said, I also don't see user identifying Firefox as blocking the
> plugin a good thing, because they'll get that prompt from Firefox itself
> saying it's Firefox (obviously), and then from every webapp, saying it's
> Firefox, at which point they'd likely say "Can't Firefox just get its act
> together and stop annoying me about that plugin already?".
True, but the "bad thing" here is that Firefox doesn't yet support sharing this information across apps, and pretending the apps are to blame for that is even worse. Firefox is in fact the agent of the dialogs and the software that doesn't have its act together, and until we fix that problem, it should take the blame.
Comment 31•11 years ago
|
||
I don't think I can provide any additional information here.
Flags: needinfo?(mshal)
Updated•11 years ago
|
Updated•11 years ago
|
Whiteboard: [apps watch list] → [apps watch list] DesktopWebRT2
Assignee | ||
Comment 32•11 years ago
|
||
This was fixed by bug 851217.
I've opened bug 930737 to discuss about when we want to use Firefox's branding and when the webapp branding.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•