428848 - Venkman can't load its <venkman-source.css>, after bug 292789 fix

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041302 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

(Noticed by Neil, while on IRC.)

1. Start SeaMonkey.
2. Load Venkman.
3. Try to open a file, in the Source Code frame.

Error Console:
{{
Security Error: Content at x-jsd:source?location=chrome%3A%2F%2Fcommunicator%2Fcontent%2Fbookmarks%2FbookmarksMenu.js&instance=25 may not load or link to chrome://venkman/skin/venkman-source.css.

Error: 
Source File: x-jsd:source?location=chrome%3A%2F%2Fcommunicator%2Fcontent%2Fbookmarks%2FbookmarksMenu.js&instance=25
Line: 2, Column: 1
Source Code:
<?xml-stylesheet type='text/css' href='chrome://venkman/skin/venkman-source.css' ?>
}}

Venkman frame:
{{
XML Parsing Error: 
Location: x-jsd:source?location=chrome%3A%2F%2Fcommunicator%2Fcontent%2Fbookmarks%2FbookmarksMenu.js&instance=25
Line Number 2, Column 1:<?xml-stylesheet type='text/css' href='chrome://venkman/skin/venkman-source.css' ?>
}}

Comment 1

[Serge Gautherie (:sgautherie)]

See bug 292789 comment 96 and bug 292789 comment 103, as hints on how to solve this.

Comment 2

[Axel Hecht [:Pike]]

Requesting this to block 1.9. This is a serious blow to debugging extensions, and, as noted, a regression.

Comment 3

[:Gijs (he/him)]

(In reply to comment #1)
> See bug 292789 comment 96 and bug 292789 comment 103, as hints on how to solve
> this.

Except Venkman doesn't have its own chrome.manifest file, so that's not going to work. Giving x-jsd privileges does not strike me as a likable idea either, quite apart from whether it is possible. Are there other solutions?

Comment 4

[Philip Chee]

> Except Venkman doesn't have its own chrome.manifest file

Err, yes it does too:

"c:\Program Files\...\SuiteRunner\extensions\{f13b157f-b174-47e7-a34d-4815ddfdfeb8}\chrome.manifest"

content venkman-ff         jar:chrome/venkman.jar!/content/venkman/ff/
content venkman            jar:chrome/venkman.jar!/content/venkman/
....
content venkman-sm         jar:chrome/venkman.jar!/content/venkman/sm/
....

Comment 5

[:Gijs (he/him)]

(In reply to comment #4)
> > Except Venkman doesn't have its own chrome.manifest file
> 
> Err, yes it does too:
> 
> "c:\Program Files\...\SuiteRunner\extensions\{f13b157f-b174-47e7-a34d-4815ddfdfeb8}\chrome.manifest"
> 
> content venkman-ff         jar:chrome/venkman.jar!/content/venkman/ff/
> content venkman            jar:chrome/venkman.jar!/content/venkman/
> ....
> content venkman-sm         jar:chrome/venkman.jar!/content/venkman/sm/
> ....
> 

No, it doesn't. I really don't care what's on your disk, but what's in CVS. Keep in mind that the fact that there's a file on your disk doesn't mean the xpi shipped with it. In this case, the chrome registry generates these manifests from the contents.rdf files:

http://mxr.mozilla.org/seamonkey/source/chrome/src/nsChromeRegistry.cpp#1597

Additionally, in your case, the SeaMonkey installer I believe ships one, which is generated from the makefile build process, because by the time SeaMonkey first runs you might not have the disk write rights to create the chrome.manifest file, so it was deemed to be better to ship with one so, you know, Venkman would actually install. See changes in bug 371671.

We can't just adapt the lines in the jar.mn that was patched there: that would only help the makefile, suiterunner generated xpi, and not the build scripts used to generate the xpi files on AMO, which don't have a chrome.manifest and rely only on contents.rdf so as to be compatible with stable SeaMonkey releases. I don't much fancy duplicating all the chrome registration info by adding a static chrome.manifest after all, either.

Comment 6

[Daniel Veditz [:dveditz]]

The manifest-generating code in nsChromeRegistry supports the "platform" attribute, we could teach it about contentaccessible (and xpcnativewrappers, and the others) too. Although at that point maybe it's easier just to include a .manifest file.

Comment 7

[Axel Hecht [:Pike]]

I'm wondering what impact x-jsd: has on the same origin policy, and if it wouldn't make more sense to make the CSS served over x-jsd: as well to get things to just work. Maybe you want to check the referrer of the load to be in x-jsd: to just fast-forward the content of the CSS.

Comment 8

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

Is Venkman broken entirely? Is it possible that we can continue to ship Firefox and publish a fix in Venkman to make this work?

Comment 9

[Axel Hecht [:Pike]]

Not broken entirely, just anything that would need or benefit from a source view displays a Yellow Screen of Death instead of a source.

Makes stepping through code or setting breakpoints et al hard. It might still work on the UI level of gdb, I never used it at that level, though.

Comment 10

[Phil Ringnalda (:philor)]

I'm not sure this helps decide severity, but in case it puts it in perspective: it's completely and utterly broken and absolutely useless for me; probably timeless and rginda are barely bothered.

Comment 11

[[e-mail]]

(In reply to comment #6)
> The manifest-generating code in nsChromeRegistry supports the "platform"
> attribute, we could teach it about contentaccessible too.

Or the manifest-generating code could simply assume contentaccessible=yes (since contents.rdf is deprecated anyway).

Comment 12

[:Gijs (he/him)]

(In reply to comment #11)
> (In reply to comment #6)
> > The manifest-generating code in nsChromeRegistry supports the "platform"
> > attribute, we could teach it about contentaccessible too.
> 
> Or the manifest-generating code could simply assume contentaccessible=yes
> (since contents.rdf is deprecated anyway).
> 

That'd work for me, but it sucks for basically all add-ons that wish to remain SeaMonkey compatible (as there is still no stable version which is built on toolkit, there is also nothing stable that knows about chrome.manifest) and don't want to expose their stuff to the web. I'm not sure if that isn't a bigger group, but would be inclined to think it probably is.

(In reply to comment #7)
> I'm wondering what impact x-jsd: has on the same origin policy, and if it
> wouldn't make more sense to make the CSS served over x-jsd: as well to get
> things to just work. Maybe you want to check the referrer of the load to be in
> x-jsd: to just fast-forward the content of the CSS.
> 

This actually seems like the most viable solution to me, though I'd be worried about caching performance. From
http://mxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#1325

I would say that this approach should work. Anyway, I'm away starting this friday until the end of April, so I don't think there'll be time for me to push this. Any volunteers?

Comment 13

[Damon Sicore (:damons)]

I'd suggest getting a patch done, ASAP, and requesting approval.  We need Venkman, but if I ask whether or not this would be the last bug holding back the release, I'd say no.  Please re-nom if you disagree.  Again, attach a patch and request approval!

Comment 14

[Benjamin Smedberg]

There is absolutely no need for this to block the FF release. The fix should take place in venkman, which can ship a chrome.manifest and a contents.rdf for compatibility.

Comment 15

[Mike Shaver (:shaver emeritus)]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #6)
> > > The manifest-generating code in nsChromeRegistry supports the "platform"
> > > attribute, we could teach it about contentaccessible too.
> > 
> > Or the manifest-generating code could simply assume contentaccessible=yes
> > (since contents.rdf is deprecated anyway).
> > 
> 
> That'd work for me, but it sucks for basically all add-ons that wish to remain
> SeaMonkey compatible (as there is still no stable version which is built on
> toolkit, there is also nothing stable that knows about chrome.manifest) and
> don't want to expose their stuff to the web. I'm not sure if that isn't a
> bigger group, but would be inclined to think it probably is.

Does that mean that SeaMonkey is going to take the change to restrict chrome-from-content somewhere in which they won't have chrome.manifest support?  I'd have thought that the chrome-from-content restriction would be in the next major from them, which would include chrome.manifest, and in the meantime they would look at contents.rdf and leave things as they were (risky).

So shipping a chrome.manifest and contents.rdf both sounds like a workable plan, given those assumptions.  A script to generate one from the other (c.r from c.m I think, given the relative eases of parsing) doesn't seem anywhere near as hard as the other cross-app goop you have to deal with (toolkit vs. xpfe?).

In other news, I propose that we do not add contentAccessible tweaking to install.js. :)

Comment 16

[[e-mail]]

(In reply to comment #15)
> So shipping a chrome.manifest and contents.rdf both sounds like a workable
> plan, given those assumptions.  A script to generate one from the other (c.r
> from c.m I think, given the relative eases of parsing) doesn't seem anywhere
> near as hard as the other cross-app goop you have to deal with
Doesn't make-jars.pl generate the .manifest from the jar.mn?

Comment 17

[:Gijs (he/him)]

(In reply to comment #16)
> (In reply to comment #15)
> > So shipping a chrome.manifest and contents.rdf both sounds like a workable
> > plan, given those assumptions.  A script to generate one from the other (c.r
> > from c.m I think, given the relative eases of parsing) doesn't seem anywhere
> > near as hard as the other cross-app goop you have to deal with
> Doesn't make-jars.pl generate the .manifest from the jar.mn?

Do I really have to explain everything twice today? Please read comment #5. Furthermore, copying (because that's basically all it is) lines from jar.mn is rather different from generating multiple contents.rdf files, in the right places, from chrome.manifest, which is what Mike is suggesting we should do.


(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #6)
> > > > The manifest-generating code in nsChromeRegistry supports the "platform"
> > > > attribute, we could teach it about contentaccessible too.
> > > 
> > > Or the manifest-generating code could simply assume contentaccessible=yes
> > > (since contents.rdf is deprecated anyway).
> > > 
> > 
> > That'd work for me, but it sucks for basically all add-ons that wish to remain
> > SeaMonkey compatible (as there is still no stable version which is built on
> > toolkit, there is also nothing stable that knows about chrome.manifest) and
> > don't want to expose their stuff to the web. I'm not sure if that isn't a
> > bigger group, but would be inclined to think it probably is.
> 
> Does that mean that SeaMonkey is going to take the change to restrict
> chrome-from-content somewhere in which they won't have chrome.manifest support?
>  I'd have thought that the chrome-from-content restriction would be in the next
> major from them, which would include chrome.manifest, and in the meantime they
> would look at contents.rdf and leave things as they were (risky).

I don't know. I don't decide anything about SeaMonkey, I'm simply cautioning against generalizing to benefit Venkman, when other consumers might want other things to happen (e.g. Adblock Plus, given Wladimir was the first to blog this change... - that said, I have no idea if ABP works on SeaMonkey, it's just a random example). Really, I'd be happy if we could generalize for Venkman... I'm just not sure that's the Right Thing to do. Benjamin certainly doesn't seem to believe so, given comment #14.

> So shipping a chrome.manifest and contents.rdf both sounds like a workable
> plan, given those assumptions.  A script to generate one from the other (c.r
> from c.m I think, given the relative eases of parsing) doesn't seem anywhere
> near as hard as the other cross-app goop you have to deal with (toolkit vs.
> xpfe?).
Actually, the other cross-app goop has the benefit of currently working (well, before this change, anyway!) and the script hasn't materialized yet. As I noted above in comment #12, I don't have the time to implement any non-trivial solution at this point. Nevermind writing a script (in what language? to be run when?) to generate multiple content.rdf files (!) from a chrome.manifest file. Given the huge amounts of love Venkman code has seen lately**, I'm not holding my breath for anyone else to do it.

> In other news, I propose that we do not add contentAccessible tweaking to
> install.js. :)
You're mean sometimes, has anyone told you that lately? :)


** that's a complaint against myself as much as others, but anyway...

Comment 18

[[e-mail]]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > So shipping a chrome.manifest and contents.rdf both sounds like a workable
> > > plan, given those assumptions.  A script to generate one from the other (c.r
> > > from c.m I think, given the relative eases of parsing) doesn't seem anywhere
> > > near as hard as the other cross-app goop you have to deal with
> > Doesn't make-jars.pl generate the .manifest from the jar.mn?
> Do I really have to explain everything twice today? Please read comment #5.
Sorry, I forgot that that jar.mn-generated manifest only works in SeaMonkey.

Comment 19

[Bruno Calvignac]

Any idea of an ETA for a fix to this problem?

Comment 20

[Jeff Beckley]

Is this a recent breakage?  I'm using Thunderbird trunk source from early April, and Venkman works fine with it.  I want to know if I need to wait until syncing to the latest sources.

As an extension author and occasional Thunderbird hacker, I'd be pretty much dead in the water without Venkman.

Comment 21

[Bruno Calvignac]

Yes, Jeff it's a recent breakage. The way I worked around it for now is to change my local copy of nsChromeRegistry::AllowContentToAccess(nsIURI *aURI, PRBool *aResult) to set *aResult to PR_TRUE and recompile.

Comment 22

[Karsten Düsterloh]

Attachment: fix trunk usage

Fix Venkman trunk usage.
(I'm not in the position to fix Venkman's ... highly tuned build ... system, though.)

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 319681 [details] [diff] [review]
fix trunk usage

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008050602 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

This fixes it for me too :-)

Yet, this is like bug 428767,
and we might want to go a step further, as in bug 428996.

Comment 24

[:Gijs (he/him)]

Attachment: Sucky patch

I don't have time to fix this properly. So let's wallpaper the hell over it for now, and we can worry about the horrible consequences later. (side question: if I have an add-on with a chrome.manifest in version X, and version X+1 doesn't, does the old one stay on the disk when I update through the add-on manager? Please tell me the answer is no :-( ... )

r?-> caillon

Comment 25

[Daniel Veditz [:dveditz]]

(In reply to comment #24)
> I have an add-on with a chrome.manifest in version X, and version X+1 doesn't,
> does the old one stay on the disk when I update through the add-on manager?
> Please tell me the answer is no :-( ... )

I'm fairly sure the update system just unpacks archives, so you can overwrite files but not really remove them.

The app-update system _does_ have a removed-files list, but not EM updates as far as I know. Using the old install.js method you could manually delete obsolete files on upgrade, but that's dead on the trunk anyway.

Comment 26

[:Gijs (he/him)]

(In reply to comment #25)
> (In reply to comment #24)
> > I have an add-on with a chrome.manifest in version X, and version X+1 doesn't,
> > does the old one stay on the disk when I update through the add-on manager?
> > Please tell me the answer is no :-( ... )
> 
> I'm fairly sure the update system just unpacks archives, so you can overwrite
> files but not really remove them.
> 
> The app-update system _does_ have a removed-files list, but not EM updates as
> far as I know. Using the old install.js method you could manually delete
> obsolete files on upgrade, but that's dead on the trunk anyway.
> 

Right, but the issue is that, if we remove the manifest again in a future version, the contents.rdf stuff should be used to recreate one, and overwrite the old one. I'm not sure if that'll happen. Sorry for not being more clear.

Comment 27

[Daniel Veditz [:dveditz]]

Mossop tells me addon updates completely delete the whole directory before unpacking the newly downloaded copy. Each update is as if it were installed fresh (apart from any user settings saved outside the extension directory).

Comment 28

[:Gijs (he/him)]

Attachment: Working XPI

PI for those not in a position to build themselves.

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 319775 [details] [diff] [review]
Sucky patch

I guess this is okay, but I'd really prefer if someone else who understands xpi/jar stuff has a look.

Comment 30

[:Gijs (he/him)]

Comment on attachment 319775 [details] [diff] [review]
Sucky patch

Ted, could you look over this, per comment 29? Thanks a lot!

Comment 31

[:Gijs (he/him)]

Checking in mozilla/extensions/venkman/resources/jar.mn;
/cvsroot/mozilla/extensions/venkman/resources/jar.mn,v  <--  jar.mn
new revision: 1.26; previous revision: 1.25
done
Checking in mozilla/extensions/venkman/xpi/makexpi.sh;
/cvsroot/mozilla/extensions/venkman/xpi/makexpi.sh,v  <--  makexpi.sh
new revision: 1.7; previous revision: 1.6
done

Comment 32

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008060404 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed

Comment 34

[(no longer active)]

Is there any chance for a new version with this patch included gets uploaded to AMO?  Otherwise, all the people who install Venkman via AMO get a broken debugger on 1.9.

Comment 35

[:Gijs (he/him)]

(In reply to comment #34)
> Is there any chance for a new version with this patch included gets uploaded to
> AMO?  Otherwise, all the people who install Venkman via AMO get a broken
> debugger on 1.9.
> 

Well, actually, I made a stupid mistake and forgot to add an extra line to keep compatibility with everything before 3.0pre. Which means that the xpi on this bug, and indeed the xpi any trunk build will generate at this time, will not work on Firefox 2. Which means I'm not putting it on AMO, and instead I'm reopening this bug.

Comment 36

[:Gijs (he/him)]

Attachment: Similarly stupid patch

According to the earlier comments on other bugs referenced by Serge, this should work. Mike, you're a Venkman peer still, and given your comments on the other bug I guess you can tell me whether this is OK or not, so the review request is for you. :-)

Comment 37

[Mike Shaver (:shaver emeritus)]

Comment on attachment 324749 [details] [diff] [review]
Similarly stupid patch

r=shaver

Comment 38

[:Gijs (he/him)]

Checking in mozilla/extensions/venkman/resources/jar.mn;
/cvsroot/mozilla/extensions/venkman/resources/jar.mn,v  <--  jar.mn
new revision: 1.27; previous revision: 1.26
done

Comment 39

[:Gijs (he/him)]

(In reply to comment #34)
> Is there any chance for a new version with this patch included gets uploaded to
> AMO?  Otherwise, all the people who install Venkman via AMO get a broken
> debugger on 1.9.
> 

As a note, a new version is waiting for review at AMO as of yesterday. They're pretty clogged up though, so I'm not sure how long it'll take. The current public version is set as compatible up to and including 3.0pre, so people who get Fx 3 don't "get a broken debugger" as is (they get no debugger at all, which may or may not be marginally better or worse).