59033 - embedding callbacks are called back into during the destruction of the widget

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

During the destruction of the embedding widget I've seen the JS status being
called back into and the network status being updated as well.  In many cases
this causes crashes since you can be calling back into dead code.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 2

[Christopher Blizzard (:blizzard)]

This patch does two things:

o It releases the event listener from the docshell attached to the primary
content shell instead of the chrome shell.  This was failing before so the event
listener was dangling.

o It removes the container window from the web browser object.  If you don't do
this then code in the docshell will try to send you updates about changes in the
object's status.

Comment 3

[Johnny Stenback (:jst)]

From looking at the patch I think there's some unnecessary code in there (minor
trivial things), the patch does:

  docShell = do_QueryInterface(contentItem);
  webProgress = do_GetInterface(docShell);

Couldn't this be done simply as:

  webProgress = do_GetInterface(contentnItem);

since docShell is not used for anything else after that QI, or did I overlooks
something?

Other than that, the patch looks fine but I can't say I know a lot about the
embedding API's so I can't really tell if the fix is ok or not on the API level...

Comment 4

[Christopher Blizzard (:blizzard)]

I didn't think that you can do a GetInterface directly from the original object.
 Now that I think about it you probably can.

Comment 5

[Christopher Blizzard (:blizzard)]

Attachment: patch 2

Comment 6

[Christopher Blizzard (:blizzard)]

Updated patch.

Comment 7

[Christopher Blizzard (:blizzard)]

I have an r=valeski on this one.

Comment 8

[Brendan Eich [:brendan]]

Cc'ing mscott to review my super-review.  This looks ok, although a little dense 
(some extra newlines before "code paragraphs", please!).  Is there any order 
dependency in removing oneself as a tree owner between browser and content tree 
items?

/be

Comment 9

[Scott MacGregor]

This looks okay to me too. I don't believe there's a problem with the order.

sr=mscott

Comment 10

[Christopher Blizzard (:blizzard)]

Checked in.  Thanks, guys.