33961 - using javascript: pseudo url in stylesheet doesn't work

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P3)

Description

[Thomas Jones]

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

[Pierre Saslawsky]

With recent debug builds, we don't get a crash but an assert in 
nsJSProtocolHandler.cpp, line 99. Reassigned to Javascript Engine.

Comment 2

[leger]

Adding crash keyword.

Comment 3

[rogerl (gone)]

trips up on assert added for #30372. There's no midi in the test case just a 
style tag with a javascript url.

Comment 4

[Johnny Stenback (:jst)]

Not required for beta2, and not very critical IMO

Comment 5

[Jesse Ruderman]

not critical?  this is a crash that can be triggered by sending text/html data 
to the browser.

Comment 6

[Johnny Stenback (:jst)]

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.

Comment 7

[Thomas Jones]

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.

Comment 8

[leger]

Adding crash to keyword field.

Comment 9

[Johnny Stenback (:jst)]

Chaging summary, removing crash keyword because this does not crash mozilla any

more.

Comment 10

[Johnny Stenback (:jst)]

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.

Comment 11

[David Baron :dbaron:]

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...

Comment 12

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 13

[Gérard Talbot]

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.

Comment 14

[Brendan Eich [:brendan]]

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

Comment 15

[Brendan Eich [:brendan]]

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

/be

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

(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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Note bug 230134

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: 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 22

[Boris Zbarsky [:bzbarsky]]

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...

Comment 23

[Darin Fisher]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Ah, good point.  Which also handles my other concern.

Comment 25

[Darin Fisher]

> 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 ;-)

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Per darin's comments

Comment 27

[Brendan Eich [:brendan]]

Expressing support for 1.7b.

/be

Comment 28

[Darin Fisher]

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

Comment 29

[Johnny Stenback (:jst)]

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

sr=jst

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

Checked in.  Still need to figure out the channel owner stuff...

Comment 31

[Serge Gautherie (:sgautherie)]

Updating:
-(U) <http://www.mindspring.com/~neuronaut/style_crash.html>, as it currently
resolves to <http://www.earthlink.net/error/404.html>.

Comment 32

[Serge Gautherie (:sgautherie)]

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

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

[Hermann Schwab]

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

Comment 35

[Serge Gautherie (:sgautherie)]

(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" :-|

Comment 36

[Boris Zbarsky [:bzbarsky]]

The fix for bug 302618 makes this completely worksforme.

Comment 37

[Peter Kehl]

This doesn't work anymore. Most likely due to new security.