61079 - wrong display and crash if change MIME type of helper app

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

DESCRIPTION:  If I change the MIME type of a helper app, this doesn't show up in
the display properly, and it leads to crashes if I try to load a file of that type.

STEPS TO REPRODUCE:
 1) Create a fresh profile
 2) Edit | Preferences | Helper Apps
 3) Add a helper app called Postscript, extension=ps,  MIME type=application/ps,
app=/usr/X11R6/bin/gv (or whatever)
 4) Click OK to get out of prefs
 5) Edit | Preferences | Helper Apps
 6) Click on "application/ps", and "Edit"
 7) Change "application/ps" to "application/postscript"
 8) Click OK
 9) Click OK
 10) Load a postscript file
 11) Edit | Preferences | Helper Apps, and try to add a handler for
application/postscript

ACTUAL RESULTS:
 7) The list of helper apps still shows "application/ps" but does not show
"application/postscript"
 10) CRASH!
 11) Even though "application/postscript" is not listed, trying to add a handler
gives an error dialog saying it is already listed

EXPECTED RESULTS:
 7) Shows application/postscript
 10) Loads postscript file correctly
 11) N/A

DOES NOT WORK CORRECTLY ON:
 * Linux, mozilla, 2000-11-22-08 (trunk)

ADDITIONAL INFORMATION:
I tried (on a second fresh profile) creating the handler with the correct MIME
type the first time, and everything worked fine.

Comment 1

[Blake Ross]

Given that flaky and broken helper app controls were one of the most prominent 
of complaints of 6.0, beta1 really shouldn't ship without this.

Comment 2

[Marcia Knous [:marcia]]

Netscape Nav triage team: this is a Netscape beta stopper.  Changing priority to 
P1 and adding Bill Law to the cc list.

Comment 3

[Mike Kaply [:mkaply]]

ccing myself and changing platforms to all.

This happens on Win, OS/2, Linux, etc.

Comment 4

[Scott MacGregor]

I thionk this bug belongs to the owner of the helper app UI. I think there's a
problem in how they are creating the data source. Either that or it's a generic
RDF in memory data source bug.

I can reproduce the crash without messing around with actually launching a
helper app. Just create  a new mime type. Dismiss the dialog, bring it up again,
change a field for the new mime type and hit okay. We crash trying to destroy
the in memory data source created by the prefs UI.

InMemoryAssertionEnumeratorImpl::~InMemoryAssertionEnumeratorImpl() line 414 +
30 bytes
InMemoryAssertionEnumeratorImpl::`scalar deleting destructor'(unsigned int 1) +
15 bytes
InMemoryAssertionEnumeratorImpl::Release(InMemoryAssertionEnumeratorImpl * const
0x00ed30d8) line 421 + 154 bytes
nsXPCWrappedNative::~nsXPCWrappedNative() line 398 + 27 bytes
nsXPCWrappedNative::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsXPCWrappedNative::Release(nsXPCWrappedNative * const 0x055ef3f0) line 71 + 31
bytes
nsXPCWrappedNative::JSObjectFinalized(JSContext * 0x058e36a0, JSObject *
0x00f47530) line 96
WrappedNative_Finalize(JSContext * 0x058e36a0, JSObject * 0x00f47530) line 783
js_FinalizeObject(JSContext * 0x058e36a0, JSObject * 0x00f47530) line 1627 + 274
bytes
js_GC(JSContext * 0x058e36a0, unsigned int 0) line 1196 + 11 bytes
js_ForceGC(JSContext * 0x058e36a0) line 931 + 11 bytes
js_DestroyContext(JSContext * 0x058e36a0, int 2) line 224 + 9 bytes
JS_DestroyContext(JSContext * 0x058e36a0) line 885 + 11 bytes
nsJSContext::~nsJSContext() line 386 + 13 bytes
nsJSContext::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsJSContext::Release(nsJSContext * const 0x058e0680) line 394 + 154 bytes
nsCOMPtr<nsIScriptContext>::~nsCOMPtr<nsIScriptContext>() line 489
nsJSContext::CallEventHandler(nsJSContext * const 0x058e0680, void * 0x00e2ed18,
void * 0x00f477a0, unsigned int 1, void * 0x0012c2d4, int * 0x0012c2d0, int 0)
line 939 + 15 bytes
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x05ee2fb4) line 154 + 64 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x05ad6f10,
nsIDOMEvent * 0x05ee2fb4, nsIDOMEventTarget * 0x05aea5b8, unsigned int 8,
unsigned int 7) line 839 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x059b22e0, nsEvent *
0x0012cb74, nsIDOMEvent * * 0x0012cb0c, nsIDOMEventTarget * 0x05aea5b8, unsigned
int 7, nsEventStatus * 0x0012cbb8) line 1721 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x05aea5b0, nsIPresContext *
0x059b22e0, nsEvent * 0x0012cb74, nsIDOMEvent * * 0x0012cb0c, unsigned int 1,
nsEventStatus * 0x0012cbb8) line 3455
PresShell::HandleDOMEventWithTarget(PresShell * const 0x059b1d60, nsIContent *
0x05aea5b0, nsEvent * 0x0012cb74, nsEventStatus * 0x0012cbb8) line 4923 + 39 bytes

Comment 5

[Scott MacGregor]

I meant to add that we are crashing because the in memory assertion enumerator
has already been deleted and we are trying to delete it again (through the xpc
wrapped native wrapper....)

re-assigning to Ben G. who wrote the uI for this. although now that I think
about it, Matt Fisher may be the owner of this now.

Comment 6

[Viswanath Ramachandran]

paul shd take this. 

Comment 7

[Paul Chen]

Funny, I don't see the crash on my mac mozilla build from today. However, I do 
see the problem where editing the pref MIME type doesn't get displayed (e.g. 
changed "application/pdf" to "application/postscript") and re-editing the mime 
type puts up the duplicated mime type dialog.

Comment 8

[Paul Chen]

Ok, I think I've found the problem. One, when editing an existing type, we're
asserting the new mime handler stuff without unasserting the old mime handler
stuff; that's why the old mapping doesn't go away. Two, the new mapping isn't
stored completely, so it doesn't show up in the list.

Comment 9

[Paul Chen]

nav triage team:

Marking nsbeta1+, mozilla0.8 milestone

Comment 10

[Paul Chen]

Attachment: set description, editable, and extensions in new handler, delete old one

Comment 11

[Paul Chen]

Just attached fix. First, we store description and file extensions in Startup() 
for use later. In Ok(), we store the description and file extensions into the new 
overrideHandler created so that it is "complete" and will show up in the mapping 
list. Then, we go and delete the old mapping by calling removeOverride.

Comment 12

[Blake Ross]

+     var gOldDesc          = null;
+     
+     var gOldExtentsions   = null;

Is it necessary to initialize these to null?  Doesn't look like it.  If not, 
please don't.  r=blake otherwise

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Why not initialize them to null? It never hurts to know what you are. 

a=[e-mail]

Comment 14

[Blake Ross]

errr...Ben and I are idiots.

+     var gOldExtentsions   = null;

`Extensions' is spelled wrong there, and you rely on it being spelled correctly 
elsewhere...

Comment 15

[Paul Chen]

Attachment: fix gOldExtentsions typo

Comment 16

[Paul Chen]

Since a typo was the only issue to the fix, I went and checked it in. Hey it's
getting kinda late, and we don't want to take any changes now with stuff like
hard drives dying, rolling blackouts, etc., do we? ;-)

Comment 17

[shrirang khanzode]

looks fine on the 0219 trunk on linux and 0.8 branch 0215. No crash is 
seen after doing the steps mentioned initially. Verified.