The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9

Status

Other Applications
Venkman JS Debugger
--
blocker
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sgautherie, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla1.9
regression
Dependency tree / graph
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [vnk-0.9.87.4])

Attachments

(3 attachments, 1 obsolete attachment)

3.19 KB, patch
Christopher Aillon (sabbatical, not receiving bugmail)
: review+
ted
: review+
Details | Diff | Splinter Review
212.47 KB, application/x-xpinstall
Details
1.01 KB, patch
shaver
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
[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' ?>
}}
(Reporter)

Comment 1

9 years ago
See bug 292789 comment 96 and bug 292789 comment 103, as hints on how to solve this.
OS: Windows 2000 → All
Hardware: PC → All

Comment 2

9 years ago
Requesting this to block 1.9. This is a serious blow to debugging extensions, and, as noted, a regression.
Flags: blocking1.9?
(Assignee)

Comment 3

9 years ago
(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

9 years ago
> 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/
....
(Assignee)

Comment 5

9 years ago
(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.
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

9 years ago
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.
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

9 years ago
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.
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.
(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).
(Assignee)

Comment 12

9 years ago
(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?
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!
Flags: blocking1.9? → blocking1.9-
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.
(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. :)
(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?
(Assignee)

Comment 17

9 years ago
(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...
(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

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

Comment 20

9 years ago
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

9 years ago
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

9 years ago
Created attachment 319681 [details] [diff] [review]
fix trunk usage

Fix Venkman trunk usage.
(I'm not in the position to fix Venkman's ... highly tuned build ... system, though.)
Attachment #319681 - Flags: review?

Updated

9 years ago
Attachment #319681 - Flags: review? → review?(gijskruitbosch+bugs)
(Reporter)

Comment 23

9 years ago
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.
(Assignee)

Updated

9 years ago
Attachment #319681 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 24

9 years ago
Created attachment 319775 [details] [diff] [review]
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
Assignee: rginda → gijskruitbosch+bugs
Attachment #319681 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #319775 - Flags: review?(caillon)
(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.
(Assignee)

Comment 26

9 years ago
(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.
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).
(Assignee)

Comment 28

9 years ago
Created attachment 320603 [details]
Working XPI

XPI for those not in a position to build themselves.
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.
Attachment #319775 - Flags: review?(caillon) → review+
(Assignee)

Comment 30

9 years ago
Comment on attachment 319775 [details] [diff] [review]
Sucky patch

Ted, could you look over this, per comment 29? Thanks a lot!
Attachment #319775 - Flags: review?(ted.mielczarek)
Attachment #319775 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

9 years ago
Blocks: 437033
(Assignee)

Comment 31

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 32

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

V.Fixed
Status: RESOLVED → VERIFIED
(Assignee)

Updated

9 years ago
Duplicate of this bug: 438048
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.
(Assignee)

Comment 35

9 years ago
(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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 36

9 years ago
Created attachment 324749 [details] [diff] [review]
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. :-)
Attachment #324749 - Flags: review?(shaver)
Comment on attachment 324749 [details] [diff] [review]
Similarly stupid patch

r=shaver
Attachment #324749 - Flags: review?(shaver) → review+
(Assignee)

Comment 38

9 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [vnk-0.9.87.4]
(Assignee)

Comment 39

9 years ago
(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).
You need to log in before you can comment on or make changes to this bug.