Closing tab while bookmarks view is shown can lead to crash [@ objc_msgSend]

RESOLVED FIXED in Camino0.9

Status

--
critical
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: moz, Assigned: me)

Tracking

({crash})

unspecified
Camino0.9
PowerPC
Mac OS X
crash

Details

(crash signature)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20040930 Camino/0.8+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20040930 Camino/0.8+

Closing a tab while the bookmarks view is open will crash Camino when the mouse
cursor is moved over the rect where the tab used to be.

Reproducible: Always
Steps to Reproduce:
1. Open 3 or more tabs (so the tab bar isn't hidden when one is closed).
2. Show the bookmarks view.
3. Press Command-W to close a tab (no visual change yet, because the bookmarks
view is open).
4. Hide the bookmarks view; note that the tab is gone as could be expected.
5. Move the mouse cursor over the rect where the tab used to be.

Actual Results:  
Camino crashed.

Expected Results:  
Not crashed.
Target Milestone: --- → Camino0.9
(Reporter)

Updated

14 years ago
Assignee: pinkerton → me
Target Milestone: Camino0.9 → ---
(Reporter)

Updated

14 years ago
Target Milestone: --- → Camino0.9
(Reporter)

Comment 1

14 years ago
Created attachment 161016 [details]
Crash log.
Confirmed.
Status: NEW → ASSIGNED

Comment 3

14 years ago
FWIW, confirming this bug under Mac OS 10.3.5, Camino nightly build 2004100308.
Created attachment 166068 [details] [diff] [review]
fixes the crash

This patch makes tab bar visibility explicit, as opposed to just looking at the
tab bar's height, which makes the code much easier to understand and gives a
mechanism for unregistering the tracking rects when the bookmark manager is
shown. The cause of the crash was that closing tabs while the tab bar was not
visible was failing to unregister the tracking rects (because the
BrowserTabView is out of the hierarchy then).

If we don't get to putting the bm manager in a tab soon, we may want to
re-evaluate the behavior of cmd-w when it's open.
(Assignee)

Updated

14 years ago
Attachment #166068 - Flags: review?(joshmoz)

Comment 5

14 years ago
Comment on attachment 166068 [details] [diff] [review]
fixes the crash

introduces a drawing problem and still crashes under the right circumstances. I
already explained it to Geoff and he can reproduce, and since its odd and sort
of complicated I don't want to write it out here.
Attachment #166068 - Flags: review?(joshmoz) → review-
Created attachment 166073 [details] [diff] [review]
fixes the problem Josh found

The problem Josh found was that the tab bar was not being rebuilt under certain
circumstances when the BrowserTabView had changed while it was hidden. This
version rebuilds the tab bar whenever it is shown after having been hidden,
which fixes that problem.
Attachment #166068 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #166073 - Flags: review?(joshmoz)

Updated

14 years ago
Keywords: crash
Summary: Closing tab while bookmarks view is shown can lead to crash. → Closing tab while bookmarks view is shown can lead to crash [@ objc_msgSend]
(Assignee)

Updated

14 years ago
Attachment #166073 - Flags: review?(Bruce.Davidson)
(Assignee)

Updated

14 years ago
Attachment #166073 - Flags: review?(joshmoz)
Attachment #166073 - Flags: review?(Bruce.Davidson)

Comment 7

14 years ago
Created attachment 166165 [details]
Alternate stack trace

Having applied the first patch it still crashes with the same set of steps to
reproduce. However the crash log is different (but the same as from an
unpatched nightly). This crash log attached.
Created attachment 166621 [details] [diff] [review]
v3

This should fix the crash Josh and Bruce saw.
Attachment #166073 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #166621 - Flags: review?(joshmoz)
(Assignee)

Updated

14 years ago
Attachment #166621 - Flags: review?(jpellico)

Updated

14 years ago
Attachment #166621 - Attachment is patch: true
Attachment #166621 - Attachment mime type: application/octet-stream → text/plain

Comment 9

14 years ago
Geoff - do you remember the comments made on IRC the other day? I should have
copied them into the bug. Waiting for a patch that addresses those...
(Assignee)

Updated

14 years ago
Attachment #166621 - Flags: review?(jpellico)
Attachment #166621 - Flags: review?(joshmoz)
Created attachment 167139 [details] [diff] [review]
address comments from IRC

Sorry... didn't forget. A combination of tree bustage and the holiday are
making me a little slow. This should address the comments from IRC.

Removed some extra brackets, added comments requested by Mike and Josh.
Attachment #166621 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #167139 - Flags: review?(joshmoz)

Updated

14 years ago
Attachment #167139 - Flags: superreview?(pinkerton)
Attachment #167139 - Flags: review?(joshmoz)
Attachment #167139 - Flags: review+
Comment on attachment 167139 [details] [diff] [review]
address comments from IRC

+  } else if (!show && mVisible) { // being hi

i assume this should be "being hidden"

sr=pink with that change.
Attachment #167139 - Flags: superreview?(pinkerton) → superreview+

Comment 12

14 years ago
landed
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Crash Signature: [@ objc_msgSend]
You need to log in before you can comment on or make changes to this bug.