using javascript: pseudo url in stylesheet doesn't work

RESOLVED WORKSFORME

Status

()

P3
normal
RESOLVED WORKSFORME
19 years ago
3 months ago

People

(Reporter: tj, Unassigned)

Tracking

(Blocks: 1 bug, {dom0})

Trunk
Future
x86
Windows NT
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7b +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

19 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m14)
BuildID:    2000033008

When using something like url ( "javascript:test()" ) for things like the
background-image: style property, mozilla crashes.	

Reproducible: Always
Steps to Reproduce:
1. Open mozilla browser.
2. Go to the provided URL.
	

Actual Results:  Mozilla crashes	

Expected Results:  Mozilla should properly execute the javascript function (in
the case of my example, that is simply an alert() popup message).	

This page works on both IE5 and Navigator 4.7.
The crash occurs in module

Comment 1

19 years ago
With recent debug builds, we don't get a crash but an assert in 
nsJSProtocolHandler.cpp, line 99. Reassigned to Javascript Engine.
Assignee: pierre → rogerl
Status: UNCONFIRMED → NEW
Component: Style System → Javascript Engine
Ever confirmed: true
QA Contact: chrisd → pschwartau

Comment 2

19 years ago
Adding crash keyword.
Keywords: crash

Comment 3

19 years ago
trips up on assert added for #30372. There's no midi in the test case just a 
style tag with a javascript url.
Assignee: rogerl → jst
Component: Javascript Engine → DOM Level 0
QA Contact: pschwartau → desale
Not required for beta2, and not very critical IMO
Severity: critical → normal
Status: NEW → ASSIGNED
Target Milestone: --- → M19

Comment 5

19 years ago
not critical?  this is a crash that can be triggered by sending text/html data 
to the browser.
By not critical I meant that it's not something that occures often at all as far
as I know.

We no longer crash when loading the testcase, marking fixed, please reopen if
it's still possible to crash mozilla with the testcase or something similar.
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Keywords: crash
Resolution: --- → FIXED
(Reporter)

Comment 7

19 years ago
Although mozilla is no longer crashing, this bug still isn't fixed.  The
javascript function is never being called.  As I have already pointed out, this
does function under Netscape 4.7 and IE 5.  Also, calling a javascript function
using the "javascript:" pseudo URL works in other places in mozilla, such as in
the href attribute of the <A> element.  So for consistency and compatibility,
this bug should be fixed.  Or, if putting the "javascript:" psuedo URL into a
style is not valid, then there should be some error message or other way of
informing the user that this is invalid instead of leaving the user wonding what
he/she did wrong.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 8

19 years ago
Adding crash to keyword field.
Keywords: crash
Chaging summary, removing crash keyword because this does not crash mozilla any

more.

Keywords: crash
Summary: using javascript: pseudo url in style crashes browser → using javascript: pseudo url in stylesheet doesn't work
This bug has been marked "future" because the original netscape engineer

workingon this is over-burdened. If you feel this is an error, that you or

another known resource will be working on this bug,or if it blocks your work in

some way -- please attach your concern to the bug for reconsideration.

Target Milestone: M19 → Future
Ugh.  I don't think javascript: URLs should work in CSS.  A URL for the 
backgroud-image property should refer to a resource that contains a 
background-image.  javascript:  URLs in HTML are bad enough...
Keywords: dom0

Updated

18 years ago
Blocks: 61019
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
Status: REOPENED → NEW

Comment 13

15 years ago
I entirely agree with comment #11. 
"javascript:" pseudo url in stylesheet does not work and should not work in
Mozilla. "javascript:" is not a valid access method (scheme) to the URL to begin
with (see RFC 1738), is almost always unhelpful and often brings accessibility
problems.

I'm all for WONTFIX-ing this bug.
javascript: URLs rule!  They pre-date data:, so they ruled even harder originally.

Why do we care about the URI scheme?  Why doesn't our fine network library make
it a matter of indifference which scheme is used, so long as the URI loads?

/be
In other words, no matter what your taste on javascript: URLs, there seems to be
a platform bug here that we should fix.

/be
The URI doesn't necessarily load, Brendan.  For example, if I do:

@import url("javascript:'div.green { color: green }'");

then nsJSThunk::EvaluateScript tries to get the notification callbacks from the
channel, there aren't any, and it bails out with NS_ERROR_FAILURE.  No idea what
the deal is with notification callbacks and whose responsibility it is to look
for them on the loadgroup.  Given what all the necko code all looks like, it
seems that it's the caller's responsibility (so EvaluateScript is buggy).  Seems
better to me if it were encapsulated in necko, though.

If I do

 body { background-image: url("javascript:alert('aaa')"); }

then I get a nice security exception:

JavaScript error: 
 line 0: uncaught exception: Permission denied to get property Window.alert

At a guess, the JS channel is screwing up its own security principal, but I'm
not really up to spending hours debugging that code right now.

So in fact, we don't care about the URI scheme past the point where we open a
channel of the right type, at which point we're outside Necko.
I don't care whose fault it is, and I'm not asking bz to stay up late debugging
-- I'm just saying don't WONTFIX this bug because you (drunclear, dbaron) don't
like javascript: URLs without filing a bug against nsJSProtocolHandler.cpp or
whatever is to blame.  Or better (and what's happened): keep this bug around to
demand a fix in dom/src/jsurl.

jst, darin: any thoughts?

/be
(In reply to comment #16)
> the deal is with notification callbacks and whose responsibility it is to look
> for them on the loadgroup.  Given what all the necko code all looks like, it
> seems that it's the caller's responsibility (so EvaluateScript is buggy).  Seems
> better to me if it were encapsulated in necko, though.

also see the comment at
http://lxr.mozilla.org/seamonkey/source/extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp#140

> 
> If I do
> 
>  body { background-image: url("javascript:alert('aaa')"); }
> 
> then I get a nice security exception:
> 
> JavaScript error: 
>  line 0: uncaught exception: Permission denied to get property Window.alert
> 
> At a guess, the JS channel is screwing up its own security principal, but I'm
> not really up to spending hours debugging that code right now.
> 
> So in fact, we don't care about the URI scheme past the point where we open a
> channel of the right type, at which point we're outside Necko.

Created attachment 143355 [details]
Partial testcase

This tests a few things.  Note that we still have a crash on the testcase in
bug 230134; I have filed bug 236881 on that.
Created attachment 143356 [details] [diff] [review]
This fixes the notification callbacks issue

This fixes up the notification callbacks mess.

The other part is because the channel does not have an owner, so is not in the
principal of the document... I'm not sure what should be going on there,
because frankly the whole owner thing is a totally undocumented hack.
Comment on attachment 143356 [details] [diff] [review]
This fixes the notification callbacks issue

Though I do still think it would make more sense for the channel to handle
talking to the loadgroup...
Attachment #143356 - Flags: superreview?(darin)
Attachment #143356 - Flags: review?(jst)

Comment 23

15 years ago
Comment on attachment 143356 [details] [diff] [review]
This fixes the notification callbacks issue

i'd recommend writing a helper function that first calls GetInterface on the
nsIInterfaceRequestor from the channel and then fails over to calling
GetInterface on the one from the loadgroup.  this is how nsHttpChannel and
others deal with callbacks, and it is the intended solution.  in this case,
your patch works, but what about the case where the channel's notification
callbacks are not null, but also do not contain the interface you are looking
for?  what if the loadgroup's callbacks do contain the interface you want?

see this code:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel
.cpp#781
Attachment #143356 - Flags: superreview?(darin) → superreview-
Comment on attachment 143356 [details] [diff] [review]
This fixes the notification callbacks issue

Ah, good point.  Which also handles my other concern.
Attachment #143356 - Flags: review?(jst) → review-

Comment 25

15 years ago
> Though I do still think it would make more sense for the channel to handle
> talking to the loadgroup...

this would require that each channel also implement nsIInterfaceRequestor, and
i'm not sure that's really what we want.  plus, it would mean that the interface
requestor you set is not the same interface requestor you get.  that also seems
wrong.

if i could do nsIChannel over, i'd maybe add a method that just works like
GetInterface ;-)
Created attachment 143359 [details] [diff] [review]
Per darin's comments
Attachment #143356 - Attachment is obsolete: true
Attachment #143359 - Flags: review?(darin)
Expressing support for 1.7b.

/be
Flags: blocking1.7b+

Comment 28

15 years ago
Comment on attachment 143359 [details] [diff] [review]
Per darin's comments

>+GetInterfaceFromChannel(nsIChannel* aChannel,
...
>+    nsresult rv = aChannel->GetNotificationCallbacks(getter_AddRefs(callbacks));
>+    if (NS_SUCCEEDED(rv) && callbacks) {
>+        callbacks->GetInterface(aIID, aResult);
...
>+        nsCOMPtr<nsILoadGroup> loadGroup;
>+        aChannel->GetLoadGroup(getter_AddRefs(loadGroup));
>+        if (loadGroup) {
>+            rv = loadGroup->GetNotificationCallbacks(getter_AddRefs(callbacks));
>+            if (NS_SUCCEEDED(rv) && callbacks) {
>+                callbacks->GetInterface(aIID, aResult);
>+            }

rv checking on GetNotificationCallbacks seems a little bit overkill.
if you got an interface pointer back, the nsCOMPtr is going to call
Release, so why not try calling GetInterface too?  there doesn't
seem to be much value in checking both NS_SUCCEEDED(rv) and 
callbacks != 0.  note: you don't check rv when getting the load
group from the channel.

r=darin
Attachment #143359 - Flags: review?(darin) → review+
Attachment #143359 - Flags: superreview?(jst)
Comment on attachment 143359 [details] [diff] [review]
Per darin's comments

sr=jst
Attachment #143359 - Flags: superreview?(jst) → superreview+
Comment on attachment 143359 [details] [diff] [review]
Per darin's comments

Checked in.  Still need to figure out the channel owner stuff...
Attachment #143359 - Attachment is obsolete: true
Comment on attachment 143355 [details]
Partial testcase


[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219] (W98SE)

No crash, no alert() nor color.


[Netscape® Communicator 4.8 : en-20020722] (W98SE)

I get {{ javascript:'.g1 { color: green; }' }} in the URL bar,
and {{ .g1 { color: green; } }} in the content area.

Not a '4xp' bug on this testcase.


[Microsoft Internet Explorer, version 5 (5.00.3105.0106) (128b, SP1)] (W98SE)

'test' and 'test2' alert(), 'g1' color,
but not 'g2' color.
Ah, forget it.  If we're going to have noise from people testing in builds it's
clearly not worth testing in, please call me once we have some consensus on how
this owner thing is supposed to work.

Comment 34

15 years ago
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040321

testing attachment 143355 [details]

no crash, two lines 'should be green' are green,
but the alerts are missing.

Javascript Console shows the same error twice:
Error: uncaught exception: Permission denied to get property Window.alert
(In reply to comment #34)
> Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040321

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040316] (W98SE)

Confirmed, between v1.7a and v1.7b:
*color: fixed :-)
*alert(): "now triggered but not yet working" :-|
The fix for bug 302618 makes this completely worksforme.
Status: NEW → RESOLVED
Last Resolved: 19 years ago13 years ago
Depends on: 302618
Resolution: --- → WORKSFORME

Comment 37

3 months ago
This doesn't work anymore. Most likely due to new security.
You need to log in before you can comment on or make changes to this bug.