347825 - [BEOS] Take more care about write_port

Status: RESOLVED FIXED

(Core Graveyard :: Widget: BeOS, defect)

Description

[Sergei Dolgov]

In theory we may fall in deadlock situation with write_port() when port is full.
Plus there is BeOS bug when port capacity reports it has some place free, when it is actually full.

So, following steps may be taken to reduce app hanging possibility:
1) Check in CallMethod, like it is done in CallMethodAsync, if there is at least 20 free positions in port, before writing there
2)Use write_port_etc with big timeout instead write_port (e.g. if port cannot be written to inside 5 seconds - we are really in trouble)
3)Use return values for nsToolkit:CallMethod*() more actively
4)Also use write_port_etc in nsAppShell::NativeEventCallback.

Comment 1

[Sergei Dolgov]

Attachment: patch

Ok, get rid of 4 cases in nsWindow::CallMethod() - those which were used by nsWindow to switch to GUI thread - 2 Create, Destroy, SetFocus. Unsure if we still need this test and switching there, but let it be.

Thus, nsToolkit::CallMethod() is unused now, but may be used in future if we need synchronous proprietery proxy. Taking it possibility in account, I rewrote it anyway, to use write_port in safer manner.

Also changed write_port to write_port_etc in nsAppShell in same way.

In addition, in some native methods in nsWindow added tests for CallMethodAsync return value - if we failed with port write, don't change flags.

r=?

Comment 2

[tqh]

Just read thru and have some quick comments:

Not sure this is ok, doesn't the create function handle switch to UI-thread in nsWindow? Do we have to call get_port_info more than once? Isn't that value constant after port creation, maybe we can even use a constant for creating and keeping track of the allowed messages? Otherwise I've no problems with the approach.

Comment 3

[Sergei Dolgov]

I'm bit confused about your first question.
How situation had look alike:
When our widget code was copied from MS Window widget code long long ago, programmers also copied 4 cases for call method to switch to UI thread if it isn't there already. Actually I don't know how frequently it happened, but out CallMethod() - synced version was used only for that.
Inspite I really don't know whether we need those switches, to clean-up/compactufy nsWindow::CallMethod(), I tried to do those switches in regular Mozilla's way, with ObjectProxy used not in StandardWindowCreate which is our "private" method, but directly in nsIWidget::Create() methods implemented in our nsWindow.

Regarding second question - I just copied code from our existing CallMethodAsync() which checks port_info every time, nothing more, only difference is that I'd set quite big (in seconds) timeout there instead 0.

Actually I like to discuss it bit more, but for me only thing which can raise some question is that that thread-switch - do we really need it at all, and does ObjectProxy perform it as intended.

Comment 4

[Sergei Dolgov]

btw, I also have some future plan to try to get rid of CallMethodAsync also with ObjectProxy, but with its "permanent" version (available for "internal API"), so we set it fot nsViewBeOS or nsWindowBeOS only once.

Didn't figure yet how to do it exacly, but maybe I have more time for that in future.

If it appears to be possible, nsAppShell will control nsWindowBeOS CurrentMessage rather than ports.

Comment 5

[Sergei Dolgov]

Little explanation of portinfo.
Some parts of that stucture are dynamic, like that "capacity" member we use.
It shows CURRENT free space at given moment in port buffer.
And I'm very very unsure that we can use for that cached pointer at portingo, which we may get at port creation. I suspect in is non-static thing and pontery syntax is used to assign/return dynamically created values.

Comment 6

[tqh]

My first question: This is the other place where switch to UI-thread is done for create: http://lxr.mozilla.org/seamonkey/source/widget/src/beos/nsWindow.cpp#490
I think the switch must be done in the create method, so do we need it inside the call-method function as well. IMO we shouldn't.

The second question, I was under the impression that the capacity was static and set on creation. So maybe we need it.

If you are still uncertain if ObjectProxy works for this maybe we should test it more before checking it in?

Comment 7

[Sergei Dolgov]

1) I still don't get it. IMHO my patch does just that - removes switch from StandardWindowCreate, removes CREATE: and CREATE_NATIVE from CallMethod, and puts it in new form (via ObjectProxy) to Create() and Create(native);

Comment 8

[tqh]

1) The answer is simple. I missed it when I looked at it before. I'll look at it some more.

Comment 9

[Sergei Dolgov]

Looks like this patch helped to fix problems in tigerdog's builds:
http://community.livejournal.com/bezilla/215464.html?replyto=1146536

Comment 10

[tqh]

Comment on attachment 232706 [details] [diff] [review]
patch

r=[e-mail]

I don't like the code duplication in create, but looks ok to me otherwise.

Comment 11

[Sergei Dolgov]

What do you think, people, about applying it to the branch, too?
Code will be alsost same, except part in nsAppShell and ObjectProxy syntax.

Comment 12

[Sergei Dolgov]

Checking in mozilla/widget/src/beos/nsAppShell.cpp;
/cvsroot/mozilla/widget/src/beos/nsAppShell.cpp,v  <--  nsAppShell.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in mozilla/widget/src/beos/nsToolkit.cpp;
/cvsroot/mozilla/widget/src/beos/nsToolkit.cpp,v  <--  nsToolkit.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.131; previous revision: 1.130
done

Comment 13

[Niels Reedijk]

I'll try to get it in before FF Beta 2 (freeze is Wednesday)

Comment 14

[Sergei Dolgov]

To nielx per comment 13.
To land in branch, it needs
1)Changing some constants names in nsGetProxyForObject calls to older version
2)Removing AppShell part of patch

Also needs little testing in branch after that

Comment 15

[Niels Reedijk]

Doug and I agreed that this one isn't going to be backported since it doesn't solve the problem that it was supposed to solve, and it needs more testing if we backport it.