373482 - Camino crashes [@ -[CHOptionSelector selectOption:] ] when changing "day" and "hour" for search of local TV schedule

Status: RESOLVED FIXED

(Camino Graveyard :: HTML Form Controls, defect)

Description

[Edward]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.8.1.2pre) Gecko/20070223 Camino/1.1b
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.8.1.2pre) Gecko/20070223 Camino/1.1b

When scanning local TV schedules for a given zipcode/television service provider, Camino crashes every time you select a new "day" and then select a new "hour" before the first selection has completely updated... 

Reproducible: Always

Steps to Reproduce:
1. goto www.tvguide.com and setup for local schedules...
2. select a new day for the search and then select the new hour before the update is finished...
3. Camino should crash... but Firefox 2.0.0.2 and Safari do not...

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can you please *attach* a crash log? http://www.caminobrowser.org/support/bugzilla/#crash

Comment 2

[Edward]

Attachment: camino crash report

Is this what you want?  I also got a prompt from the Mozilla agent but don't have time to do that now... I can try it later, if need be...

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)

> Is this what you want?
In the future, please convert it to plain text first (Format: Make Plain Text), so we don't have to download it to read it ;)

I see this too, but with a different (and more accurate?) frame 0.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Crash log

Here are my STR, which might be different from comment 0:

1. Load http://www.tvguide.com/listings/setup/localize.aspx
2. Input zip code and submit
3. Choose cable provider
4. Wait for initial listings to load
5. Choose a new date
6. Choose a new time before the listings have completely loaded again

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is also showing up a few times in Talkback.

Comment 6

[froodian (Ian Leue)]

nsIDOMHTMLSelectElement* mSelectElt in CHSelectHandler (né CHClickListener) should be a strong ref.  When investigating this, we should also investigate what happens with the option children (if they'll get retained when we retain the select or not, since if not that'll still be a crash source)

Comment 7

[Stuart Morgan]

And we need to investigate whether or not holding a strong ref to a page element that may be removed from the page is even a good idea at all; I have no idea how, e.g., JS references would be resolved for elements that aren't part of the page's DOM anymore.

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.1

This basically add strong references as froodian suggested.

The comptr held on by CHOptionSelector should be release when that object goes away (since the construction of CHOptionSelector is auto release).

The option_pool goes away when ShowNativeMenuForSelect leaves scope.  This will clear the array.

Comment 9

[Doug Turner (:dougt)]

i should point out that I am a serious objective-c noob and adding that nsCOMPtr to the interface added these warnings to the build:

/Camino.build/Development/Camino.build/Objects-normal/i386/CHSelectHandler.o
/Users/dougt/builds/camino/mozilla/camino_dbg/camino/src/embedding/CHSelectHandler.mm:69: warning: type `nsCOMPtr<nsIDOMHTMLSelectElement>' has a user-defined constructor
/Users/dougt/builds/camino/mozilla/camino_dbg/camino/src/embedding/CHSelectHandler.mm:69: warning: type `nsCOMPtr<nsIDOMHTMLSelectElement>' has a user-defined destructor
/Users/dougt/builds/camino/mozilla/camino_dbg/camino/src/embedding/CHSelectHandler.mm:69: warning: C++ constructors and destructors will not be invoked for Objective-C fields

I am not sure how much of this is just bitching, or something that needs to be dealt with.

Comment 10

[Stuart Morgan]

Comment on attachment 281594 [details] [diff] [review]
patch v.1

You can't have a C++ class as a direct member of an ObjC object; its constructor and destructor will never be called.

Did you look into my concerns in comment 7, to verify that holding a strong ref will result in reasonable behavior? I looked briefly at the select menu code in core at some point for another bug, and as far as I could tell they seemed to do something more complex with an indirection object that could be nulled out (although I didn't see what the mechanism was for that happening, so I may have misunderstood).

Comment 11

[Doug Turner (:dougt)]

okay, so it sounds like that means the mSelectElt would be leaked in this case.  I am wondering if we even need to hold on to that reference.  I tried without the nsCOMPtr and I don't crash. 

regarding comment 7, i am not sure.  

Comment 12

[Doug Turner (:dougt)]

mass reassigning to nobody.

Comment 13

[Doug Turner (:dougt)]

Attachment: patch v.2

this simply refcounts the options before putting them in the native menu items.  confirmed that holding onto the option elements after the select is destroyed is not harmful.

Comment 14

[Stuart Morgan]

Comment on attachment 284051 [details] [diff] [review]
patch v.2

This isn't the right patch for this bug.

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 16

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 17

[Doug Turner (:dougt)]

Attachment: patch v.5

Comment 18

[Stuart Morgan]

Comment on attachment 284060 [details] [diff] [review]
patch v.5

>+  // We need to addref all of the option elements that we set on the menu,
>+  // then auto release them when we leave this function. see bug: 373482

While not crashing is certainly better than crashing, given that this solution has the potential to get sites into states that they might reasonably expect to be impossible I'd like a TODO here explaining that this is a workaround, and that we should actually do is dismiss the menu if the <select> goes away, or at least do nothing when a removed option is selected.

>+  if (NS_FAILED(rv))
>+	return rv;

This should be spaces, not a tab.

Comment 19

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 284060 [details] [diff] [review]
patch v.5

sr=pink with stuart's comments addressed.

Comment 20

[Doug Turner (:dougt)]

I referenced this bug in the comment.  People can read all of the details here.  

I will fix the ws issues.

Comment 21

[Doug Turner (:dougt)]

Checking in CHSelectHandler.mm;
/cvsroot/mozilla/camino/src/embedding/CHSelectHandler.mm,v  <--  CHSelectHandler.mm
new revision: 1.2.2.2; previous revision: 1.2.2.1
done


Checking in CHSelectHandler.mm;
/cvsroot/mozilla/camino/src/embedding/CHSelectHandler.mm,v  <--  CHSelectHandler.mm
new revision: 1.3; previous revision: 1.2
done

Marking bug fixed.  

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We should think about taking this when we do 1.5.3.  Thanks for the fix!

Comment 23

[Stuart Morgan]

(In reply to comment #20)
> I referenced this bug in the comment.  People can read all of the details here.

Yes, but they are very unlikely to. We not infrequently mention bugs in comments, and it rarely means that it's a workaround that we don't consider optimal, so nothing about that comment would suggest that we should really be doing something very different if possible.

Please add the TODO as requested.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: backported to the 15branch

Should we want to take this in 1.5.4, here's the patch "backported" to 1_5 branch (hunk 1 didn't apply cleanly for reasons beyond me--and the filename changed).

Comment 25

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed the backport on the CAMINO_1_5_BRANCH for Camino 1.5.4.