32341 - Crash attempting to save Javscript: url

Status: VERIFIED FIXED

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

Description

[Bill Law]

To reproduce:
1. Go to the page specified by this bug's URL
(http://www.spe.sony.com/movies/timecode/trailer.html).
2. Right mouse click on "hi res trailer" link.
3. Choose Save Link As... from the context menu.

Crashes.

This is another flavor of the problem reported in bug 23690.  My fix for that
bug filtered the problem-causing context menu choices for javascript URLs but I
didn't catch alternative spellings.

Further investigation reveals that a better fail-safe fix is to avoid the crash
within the JS protocol handler.  Here's a patch that does that:

Index: nsJSProtocolHandler.cpp
===================================================================
RCS file: /cvsroot/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,v
retrieving revision 1.38
diff -r1.38 nsJSProtocolHandler.cpp
200a201
>     NS_ENSURE_ARG_POINTER(notificationCallbacks);

This problem exists on the tip and the beta1 branch.  I will be applying that
fix on the tip.  I'm marking the bug beta1 so we can assess whether it's PDT+
(and should be fixed on the beta1 branch).

Note that the fix is about as low-risk as you can get (simply returns an error
when presented a null pointer rather than immediately turn around and try to
dereference that pointer).

Comment 1

[Bill Law]

Adding beta1 and crash keywords.  Also cc:ing [e-mail] since he reported
this originally.

Comment 2

[Jim Roskind]

Hmm... a very tempting fix indeed.
In all cases where the code would have *any* impact, the browser is about to
crash, correct?
If we want this in for beta1, I'd say we have to have a resolution to facilitate
a checkin by Sunday night.
Rick: comments?

The only argument (and it is a good argument) against the checkin is that this
is a very odd thing to do (re: save of JS url), and hence it should have no
impact on 99.9999% of the users (minimum).
My bet is that we'll just say "beta 2."

Comment 3

[don]

Bill, if this is the single-line check-for-null fix then we should get this in
(at least) the trunk as soon as possible.

Comment 4

[leger]

Putting on PDT- radar for beta1.

Comment 5

[don]

If it's waited this long, it can wait until M16 ...

Comment 6

[Prashant Desale]

Adding nsbeta2 to keywords.

Comment 7

[Michael La Guardia]

Reassigning to Clayton.  Clayton, PDT asks that you find someone to land the one 
line fix enclosed herein.  Marking nsbeta2+ to stop the crash.

Comment 8

[clayton]

Move to Roger

Comment 9

[rogerl (gone)]

-->DOM0 component owner

Comment 10

[Johnny Stenback (:jst)]

This doesn't crash my fresh optimized linux build but my WinNT debug build from
a couple of days ago crashes, updating tree to test again...

Comment 11

[Johnny Stenback (:jst)]

This is as far as I can tell fixed, I tried on both linux (optmized) and WinNT
(debug), buth from today, and I get the "Downloading file" dialog and then a
dialog saying "Unknown error". No crash. I do see an assert on WinNT from

    NS_ENSURE_TRUE(callbacks, NS_ERROR_FAILURE);

in nsJSProtocolhandler.cpp, line 100.

Looks like the above patch was checked in at some point, and bonsai tells me
that I checked in the patch, but I did that to fix bug 30372 where some odd
plugin caused a crash in the same place.

I'm marking this fixed since I can't reproduce this in mozilla any more.

Comment 12

[Prashant Desale]

Verified with 2000-02-22-08.