If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

BeOS build broken after landing of nsIThreadManager patch 326273

RESOLVED FIXED

Status

()

Core
XPCOM
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Doug Shelton, Assigned: Sergei Dolgov)

Tracking

Trunk
x86
BeOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
Component: General → XPCOM
Product: Mozilla Application Suite → Core
Version: unspecified → Trunk
(Reporter)

Comment 1

12 years ago
Created attachment 221628 [details]
Console output of bustage
Status: UNCONFIRMED → NEW
Ever confirmed: true
Depends on: 326273

Updated

12 years ago
Blocks: 326273
No longer depends on: 326273
(Assignee)

Comment 2

12 years ago
Created attachment 221744 [details] [diff] [review]
qucky dirty patch

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

12 years ago
Created attachment 221903 [details] [diff] [review]
working patch

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

12 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

12 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

12 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

12 years ago
Created attachment 221911 [details] [diff] [review]
reduced working patch

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

12 years ago
Comment on attachment 221911 [details] [diff] [review]
reduced working patch

r?
Attachment #221911 - Flags: review?(thesuckiestemail)
(Assignee)

Comment 9

12 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

12 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

12 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

12 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

12 years ago
Created attachment 221933 [details]
Console output of Firefox @ 100% CPU utilisation

Firefox (built without places) displays same high CPU utilization.  Repeated output during looping is:
Retrived ni = (nil)
CMA
TK-GI
PNNE
(Assignee)

Comment 14

12 years ago
per comment 13:
those are my debug messages from nsToolkit and nsAppShell
(Assignee)

Comment 15

12 years ago
bookmark for reading
https://bugzilla.mozilla.org/show_bug.cgi?id=337550#c17

Comment 16

12 years ago
I guess the review was cancelled? Do we want to get something checked in to allow building?
(Assignee)

Comment 17

12 years ago
I will submit new one today, with two mistakes in AppShell corrected and #define DEBUG removed
(Assignee)

Comment 18

12 years ago
Created attachment 221960 [details] [diff] [review]
working patch, no more 100% CPU load

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

12 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

12 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

12 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

12 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

12 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
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.