337489 - BeOS build broken after landing of nsIThreadManager patch 326273

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Shelton]

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

Comment 1

[Doug Shelton]

Attachment: Console output of bustage

Comment 2

[Sergei Dolgov]

Attachment: 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

Comment 3

[Sergei Dolgov]

Attachment: working patch

ok, there are lot of comments inside AppShell.cpp
and code works for me (at least with splash-screen disabled).

Comment 4

[Sergei Dolgov]

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

Comment 5

[Sergei Dolgov]

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

Comment 6

[Sergei Dolgov]

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

Comment 7

[Sergei Dolgov]

Attachment: 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.

Comment 8

[Sergei Dolgov]

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

r?

Comment 9

[Sergei Dolgov]

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.

Comment 10

[Sergei Dolgov]

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

Comment 11

[Sergei Dolgov]

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()

Comment 12

[Sergei Dolgov]

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

found bunch of little issues.

Comment 13

[Doug Shelton]

Attachment: 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

Comment 14

[Sergei Dolgov]

per comment 13:
those are my debug messages from nsToolkit and nsAppShell

Comment 15

[Sergei Dolgov]

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

Comment 16

[tqh]

I guess the review was cancelled? Do we want to get something checked in to allow building?

Comment 17

[Sergei Dolgov]

I will submit new one today, with two mistakes in AppShell corrected and #define DEBUG removed

Comment 18

[Sergei Dolgov]

Attachment: 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.

Comment 19

[Sergei Dolgov]

Comment on attachment 221960 [details] [diff] [review]
working patch, no more 100% CPU load

r?
(there is still qustion about best timeout value)

Comment 20

[Sergei Dolgov]

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

Comment 21

[Sergei Dolgov]

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

[tqh]

Comment on attachment 221960 [details] [diff] [review]
working patch, no more 100% CPU load

r=[e-mail]

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.

Comment 23

[Sergei Dolgov]

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