480797 - window closing before selecting item from context menu crashes Camino [@ nsCOMPtr<nsIDOMNode>::get_DerivedSafe][@ -[BrowserWindowController copyImageLocation:] ], etc.

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[hhaamu]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.8pre) Gecko/2009022317 Camino/2.0b2
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.8pre) Gecko/2009022317 Camino/2.0b2

The URL opens a new browser window with image. The pop-up is designed to close at an onmousedown event. Right-clicking displays context menu, but triggers onmousedown event (which closes window).

Selecting an item from the (dangling) context menu crashes browser.

Reproducible: Always

Steps to Reproduce:
1. Click on the image to trigger pop-up
2. Right-click on image in the pop-up
3. [pop-up closes due to onmousedown event]
4. Select "Copy image location"
5. Crash
Actual Results:  
a short beachballing, and then crash


Related: bug 168065

Comment 1

[hhaamu]

Attachment: testcase

Testcase. Can probably be simplified a bit.

Comment 2

[philippe (part-time)]

Attachment: crash log

btw, this is not new to Camino 2.0b2, I can go back to the 20080915 nightly build. Will check later with older builds.

Minefield & Fx 3 don't crash (the context menu is not triggered).

Comment 3

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

This is probably a dupe of bug 342672 (and maybe bug 183586, which could have been reopened by later appshell changes).  It's certainly the same basic bug as those two and bug 168065.

Comment 4

[Chris Lawson (gone)]

Can the code associated with window closure be modified to ensure that any context menus on that window are closed as well? That seems like it would fix this.

What we really need is a fix for bug 168065, though.

Comment 5

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

Since the other appshell+context menu bugs are still, in fact, fixed, confirming this.

Oddly enough, "Copy Image" does *not* trigger the crash.

As an alternate strategy to fixing bug 168065, can we ensure that all of our context menu items check to make sure the document/image/selection/whatever that they think they're operating on is still around?

Comment 6

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

Attachment: debug crash log

We get some Gecko at the top the thread in a debug build, fwiw.

Comment 7

[Chris Lawson (gone)]

Attachment: somewhat better testcase

This testcase makes the image a link as well, so we get all the link CM items (which were also mostly crashing here).

I'm about to post a patch that implements comment 5 and fixes all the crashes.

Comment 8

[Chris Lawson (gone)]

Attachment: fix v1.0

This implements comment 5. I can't figure out how to do comment 4, but these null-checks seem like a good idea anyway, even if the other bug gets fixed and/or someone figures out how to do comment 4.

Comment 9

[Chris Lawson (gone)]

This is probably an issue on branch as well, so do we want this for 1.6.7?

Comment 10

[philippe (part-time)]

(In reply to comment #9)
> This is probably an issue on branch as well, so do we want this for 1.6.7?
When I tested this with the patch in comment 0, I couldn't get 1.6.6 to crash. It does crash with the second testcase and choosing 'save image as…'.

Comment 11

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

Attachment: more extensive testcase

Besides "Copy Address" which I've tested to crash, what items from other context menus also need to be fixed?

Comment 12

[Chris Lawson (gone)]

With the patch in comment 8, I can't get a crash with any of the CMs that can be triggered with the new testcase from comment 11.

Comment 13

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

Attachment: branch version

Thanks to some investigation from hendy, here's the branch version of the patch.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 365300 [details] [diff] [review]
fix v1.0

sr=pink

Comment 15

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

Landed on cvs trunk and the MOZILLA_1_8_BRANCH for 1.6.7.