Closed
Bug 337489
Opened 18 years ago
Closed 18 years ago
BeOS build broken after landing of nsIThreadManager patch 326273
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: doug, Assigned: sergei_d)
References
Details
Attachments
(3 files, 3 obsolete files)
User-Agent: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20060505 Minefield/3.0a1 Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20060505 Minefield/3.0a1 SeaMonkey will not build from current trunk. Appears to be due to changes from 326273. Reproducible: Always
Reporter | ||
Updated•18 years ago
|
Component: General → XPCOM
Product: Mozilla Application Suite → Core
Version: unspecified → Trunk
Reporter | ||
Comment 1•18 years ago
|
||
Updated•18 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•18 years ago
|
Depends on: nsIThreadManager
Updated•18 years ago
|
Blocks: nsIThreadManager
No longer depends on: nsIThreadManager
Assignee | ||
Comment 2•18 years ago
|
||
That wasn't my day today, bustages in sqlconnect, first time seen bustage in XForms (should it really be built on beos with simple --enable-extensions????), so i was unable to test, debug, fix and polish anything). Hope tomorrow, if my build is OK at last, better version will be submitted. That one is just for reference. Don't take it seriosly - it just allows beos widgets to build
Assignee | ||
Comment 3•18 years ago
|
||
ok, there are lot of comments inside AppShell.cpp and code works for me (at least with splash-screen disabled).
Assignee: general → sergei_d
Attachment #221744 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•18 years ago
|
||
As ConsumeRedundantMouseEvents is gone, scrolling is quite slow. So we either need to restore this function new implementation (but it didn't help enough even in former one) or land at last some of enumerous version of MouseMove patch
Assignee | ||
Comment 5•18 years ago
|
||
For non-BeOS reviewers. Current implenentation of BeOS AppShell: static gNotified = true; PRBool nsAppShell::ProcessNextNativeEvent(PRBool mayWait) { bool gotMessage = false; if (!message_port.IsEmpty()) { if (mayWait) { gNotified = false; while(gNotified); } bool gotMessage = true; read_message_from_port(message) dispatch_message(message); } return gotMessage; } void nsAppShell::ScheduleNativeEventCallback() { // Unblocking. Previously done via _pl_NativeNotify() gNotified = true; } P.S. for BeOS persons - forgot to include bool gotMessage = true; in patch
Assignee | ||
Comment 6•18 years ago
|
||
Ok, thanks to biesi notice. mayWait and ScheduleNativeEventCallback parts appeared to be deadly wrong. So it worked, but mayWait flag was totally ignored. invertic logic to what I intended actually: while(!gNotified); allows Mozilla to start but then blocks it for ever
Assignee | ||
Comment 7•18 years ago
|
||
ProcessNextNativeEvent() now totally ignores mayWait, Return value for ProcessNextNativeEvent() is assigned properly. Potential leak with newitem fixed. ScheduleNativeEventCallback() is empty Until we understand if and when we need it together with mayWait and how to implement. Also identation fixed, where I found it inconsistent. I think it is good candidate for checkin as bustage fix, until tqh and big gurus have time for implementation of better solution.
Attachment #221903 -
Attachment is obsolete: true
Assignee | ||
Comment 8•18 years ago
|
||
Comment on attachment 221911 [details] [diff] [review] reduced working patch r?
Attachment #221911 -
Flags: review?(thesuckiestemail)
Assignee | ||
Comment 9•18 years ago
|
||
for tqh Currently I see call for ScheduleNativeEventCallback() only once, at very early stage of application bootstrap, before first window created. Maybe we need it in future when have more code in be_app loop in bootstrap or when moving be_app-Run() in AppShell.
Assignee | ||
Comment 10•18 years ago
|
||
first results with this temporary patch - it eats CPU. Looks like that permanent poll is happening somwehere. Will investigate if it happens in our AppShell
Assignee | ||
Comment 11•18 years ago
|
||
Had look inside appshell with debug printout enabled. At least it is clear that that 100% CPU usage (50x50 on my dual CPU, btw) is NOT result of permanent endless call of ProcessNextNativeEvent()
Assignee | ||
Comment 12•18 years ago
|
||
Comment on attachment 221911 [details] [diff] [review] reduced working patch found bunch of little issues.
Attachment #221911 -
Flags: review?(thesuckiestemail)
Reporter | ||
Comment 13•18 years ago
|
||
Firefox (built without places) displays same high CPU utilization. Repeated output during looping is: Retrived ni = (nil) CMA TK-GI PNNE
Assignee | ||
Comment 14•18 years ago
|
||
per comment 13: those are my debug messages from nsToolkit and nsAppShell
Assignee | ||
Comment 15•18 years ago
|
||
bookmark for reading https://bugzilla.mozilla.org/show_bug.cgi?id=337550#c17
Comment 16•18 years ago
|
||
I guess the review was cancelled? Do we want to get something checked in to allow building?
Assignee | ||
Comment 17•18 years ago
|
||
I will submit new one today, with two mistakes in AppShell corrected and #define DEBUG removed
Assignee | ||
Comment 18•18 years ago
|
||
So new patch. Uses mayWait formally, though, dunno if it does it in reality. No more 100% CPU load. All that appeared to be possible after replacing read_port() (for non-beosers - thing which blocks thread until port queue is empty) with read_port_etc(,timeout) (for same public - it blocks too, but only for timeout). ScheduleNativeEventCallback is still to implement, if needed.
Attachment #221911 -
Attachment is obsolete: true
Assignee | ||
Comment 19•18 years ago
|
||
Comment on attachment 221960 [details] [diff] [review] working patch, no more 100% CPU load r? (there is still qustion about best timeout value)
Attachment #221960 -
Flags: review?(thesuckiestemail)
Assignee | ||
Comment 20•18 years ago
|
||
adding for reference my thoughts from our blog: ---------------------- Actually logic, using our functions, must look like if (mayWait) //blocking allowed { read_port(); dispatch; } else { if (port_count() > 0) read_port(); dispatch; } but it didn't work, untile read_port was replaced with read_port_etc(timeout ) Maybe it really needs ScheduleNativeEventCallback() implementation, but maybe our current message-chain architecture is "too special" (Mac ports, Cocoa especially, also has troubles atm with this update). As I see things, we use bit inverted approach, in comparison to win/gtk ports. In those ports functions in AppShell hook into upper-level application message loop, for us it is something like BApplication/BWindow::DispatchMessage(). ProcessNextNativeEvent() in those ports works, in our terms, with BApplication/BWindow::MessageQueue(), redispatching its messages, so those can reach widget/window level <b>by natural native way</b> - to be converted there into mozilla messages and dispatched to Gecko. For our case, we first catch those messages in widget/windows via BWindow/BView::Looper() hooks (so no intervention at higher level (unlike it happens in other ports), then resending those via toolkit callmethods to AppShell, where those are "processed" with that ProcessNextNativeEvent() and "sent" again to widget single huge artifical hook function CallMethod() - and from there those fot converted and dispatched to Gecko. So we already have lot of callback-like scheduled activity with all our nsTookit tricks
Assignee | ||
Comment 21•18 years ago
|
||
So I thing, as we don't handle here really-native loop, that if (mayWait) read_port(); don't work because read_port() blocks same thread where those no-more-native messages should pass-through - toolkits CallMethod*s. So it cannot be unblocked. Maybe even with current model we need to try implement if future separate BLooper instead.
Comment 22•18 years ago
|
||
Comment on attachment 221960 [details] [diff] [review] working patch, no more 100% CPU load r=thesuckiestemail@yahoo.se IIRC I think everything with the syncsem is unused now. The only reason the sync-sem was there was because plevent or nsAppShell could open the port. Ofc it had a bigger part before that.
Attachment #221960 -
Flags: review?(thesuckiestemail) → review+
Assignee | ||
Comment 23•18 years ago
|
||
Checking in mozilla/widget/src/beos/nsAppShell.cpp; /cvsroot/mozilla/widget/src/beos/nsAppShell.cpp,v <-- nsAppShell.cpp new revision: 1.32; previous revision: 1.31 done Checking in mozilla/widget/src/beos/nsAppShell.h; /cvsroot/mozilla/widget/src/beos/nsAppShell.h,v <-- nsAppShell.h new revision: 1.13; previous revision: 1.12 done Checking in mozilla/widget/src/beos/nsToolkit.cpp; /cvsroot/mozilla/widget/src/beos/nsToolkit.cpp,v <-- nsToolkit.cpp new revision: 1.25; previous revision: 1.24 done Checking in mozilla/widget/src/beos/nsToolkit.h; /cvsroot/mozilla/widget/src/beos/nsToolkit.h,v <-- nsToolkit.h new revision: 1.7; previous revision: 1.6 done Checking in mozilla/widget/src/beos/nsWidgetFactory.cpp; /cvsroot/mozilla/widget/src/beos/nsWidgetFactory.cpp,v <-- nsWidgetFactory.cpp new revision: 1.39; previous revision: 1.38 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•