228623 - [patch] Bookmark update code causes periodic hangs in camino

Status: RESOLVED WONTFIX

(Camino Graveyard :: Bookmarks, defect)

Description

[Simon Fraser [no longer active]]

I keep having Camino hang on me for periods of about 30 seconds. ThreadViewer
shows this stack:

mach_msg_trap
mach_msg
_lookup_all_secure
_lookup_all
gai_lookupd
getaddrinfo
__SCNetworkReachabilityGetFlags
SCNetworkReachabilityGetFlags
SCNetworkReachabilityByName
-[Bookmark hostIsReachable]
-[Bookmark checkForUpdate]
__NSFireTimer
(runloop stuff)

This is bad. I think Camino should use necko to do bookmark checking, which will
push stuff onto a separate thread, and avoid stalling the UI.

Comment 1

[Noemi]

Perhaps this is what was responsible for Camino hanging on me every minute or
two for a few seconds while my internet connection was down this afternoon? 
It's not something that ever happens when the connection is available.  

Comment 2

[Mike Pinkerton (not reading bugmail)]

damnit, we need to fix this. sbwoodside, didn't we test this before landing the
big bookmarks patch?

Comment 3

[S Woodside]

Yes, I did test this with a modem connection. When the connection was
disconnect, it did not try to connect or freeze or hang. I tested on 10.2.
What's the system? What's the type of network connection?

Comment 4

[Simon Fraser [no longer active]]

Dual G4 on the end of a DSL line.

Comment 5

[S Woodside]

i'll look at it.

What OS version?

Comment 6

[Simon Fraser [no longer active]]

10.3.2 I think.

Comment 7

[S Woodside]

I'm unable to reproduce this. I'm running 10.2.8. Maybe it's a panther-only bug?
Here's what I did:
- load up router configuration page
- disconnect PPPoE
- surf pages on my localhost server for a while
- I didn't see any hangs or freezes

Comment 8

[Simon Fraser [no longer active]]

This is the kind of bug where you can't expect to be able to reproduce every
networking issue that might affect it. It's enough to know that the problem
occurred to force you to seek alternative implementations. As I mentioned above,
I think necko should be used here, instead of alternative APIs.

Comment 9

[S Woodside]

This code is mainly in doHTTPUpdateRequest in Bookmark.mm. It's implemented
using Core Foundation network library CFNetwork services. I think CFNetwork is
thread safe so a simpler workaround is probably to stick it on an NSThread and
use cocoa threads to avoid the hang.

I don't know if there is some gain in converting to necko. Necko has both
synchronous and async modes too, so it's the same amount of work to make this
work multithreaded either way I think.

Anyway another idea I had was to implemented a necko async wrapper in cocoa, so
that we could make async necko calls using the obj-c threads api.

Comment 10

[S Woodside]

Can you still reproduce this hang? I've got a patch that makes HTTP update
checks async. But I have no way to verify if it fixes the problem since I can't
reproduce it .......

Comment 11

[S Woodside]

Attachment: test patch

Try this out. I'll need to clean it up if it works.

Comment 12

[S Woodside]

Attachment: real test patch

whoops, use this one. disregard the previous. I left in some spurious diffs on
my project file by mistkae.

Comment 13

[Ludovic Hirlimann [:Usul]]

Will work on it ASAP, tonight, tomorow night ...

Comment 14

[Ludovic Hirlimann [:Usul]]

Comment on attachment 138725 [details] [diff] [review]
real test patch

Giving R but needs to be cleaned up . Too much logging is done in the current
patch.

Comment 15

[Ludovic Hirlimann [:Usul]]

Sbwoodside are you still working on Camino ? do you plan to update your patch ?

Comment 16

[Jasper]

Attachment: Crash log.

Ok I just had a randow crash while I was dragging a text clipping to a tab to
open  a website. Camino just crashed, plof.

looking at the crash log, it directs me to the bookmark updating code since
there are some direct link in it.

2   org.mozilla.navigator	0x0008b100 -[BookmarkViewController
reloadDataForItem:reloadChildren:] + 0x8c (BookmarkViewController.mm:1198)
3   org.mozilla.navigator	0x0008ba3c -[BookmarkViewController
bookmarkChanged:] + 0x68 (BookmarkViewController.mm:1351)
4   <<00000000>>	0x909f793c 0 + 0x909f793c
5   <<00000000>>	0x901aa680 0 + 0x901aa680
6   <<00000000>>	0x901af090 0 + 0x901af090
7   org.mozilla.navigator	0x0007b064 -[BookmarkItem itemUpdatedNote] +
0xa4 (BookmarkItem.m:234)
8   org.mozilla.navigator	0x0006e5d0 -[Bookmark setNumberOfVisits:] +
0xa0 (Bookmark.mm:203)
9   org.mozilla.navigator	0x0006ebf0 -[Bookmark urlLoadCheck:] + 0x240
(Bookmark.mm:256)

Comment 17

[Simon Fraser [no longer active]]

Yeah, I still see periodic hangs (when there are network issues), and see
reports by others on the mailing list. Simon?

Comment 18

[Jasper]

Simon W could you update please?

Comment 19

[Stuart Morgan]

Pushing back to 0.9, since auto-update will be disabled in 0.8 (and only
re-enableable through a hidden pref).

Comment 20

[David Haas (gone, not reading mail)]

Attachment: simple patch

This patch pushes the "hostIsReachable" function onto a background
thread.  If it can find the host, it calls on the main thread the
doHTTPUpdateCheck.  

This patch will break Camino on 10.1, which I believe is OK at
this point.  I think some calls to "RunLoopMessenger" rather than
performSelectorOnMainThread would bring back 10.1 compatability.

Based on the threadviewer results, hostIsReachable is the main
culprit for the intermittant hangs.  I think (but don't know for
sure) the rest of the CFNetwork stuff is well behaved on the
main thread.  If it turns out more CFNetwork stuff is problematic,
the original version of the big bookmark patch had mostly-working
background thread calls for the bookmark updating, so you might
want to look there.

It would be nice if a couple people would try this patch out,
turn on the bookmark checking, and see if the hangs go away.

Comment 21

[Josh Aas]

broken bookmark detection code is getting ripped out, so marking this bug as wontfix