601282 - Sometimes fennec's window is black when started the DEBUG intent

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Start fennec with am start -a org.mozilla.gecko.DEBUG -n org.mozilla.fennec/org.mozilla.fennec.App
 (2) Click "launch"
Most of the time fennec doesn't draw, just appears as a black window.

When this happens, I've found that changing orientation seems to make fennec re-render.  I don't much care about the DEBUG intent, but this seems to indicate a race condition that could bite us in normal operation.

Comment 1

[Alex Pakhotin (:alexp)]

The problem seems to be because of the different order of some function calls for DEBUG workflow.

Normally we launch Gecko from onCreate, then the GeckoApp activity handles onStart and onResume, where we raise the flag surfaceView.mSurfaceNeedsRedraw. After that at some point the system calls GeckoSurfaceView.surfaceChanged where according to the flag the redraw is scheduled.

In DEBUG mode the onStart, onResume, and GeckoSurfaceView.surfaceChanged calls happen before we launch Gecko (which is done later on a button click), and apparently this screws up the drawing. Another call to GeckoAppShell.scheduleRedraw() right after (or before) the launch doesn't seem to help. I don't see an easy fix at the moment - tried some hacks, but they didn't work right away. This needs to be investigated further to fix the DEBUG flow, but I believe we shouldn't have such problem in normal operation, where the functions are called in a proper order. They are in the same thread, so there is no race condition.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I just got a black screen when starting up normally on device.  Not sure of it's related to this.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This happened to me again on device.  I'm using a local build from m-c f9f10c04dceb and m-b cdb24af322a8.  I hadn't seen this happen during normal startup before new dlopen.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Chris: I think you're seeing bug 604090 unless you're starting in a DEBUG intent.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm worried these are symptoms of the same problem (comment 0).  Might be different problems though.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 7

[Michael Wu [:mwu]]

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

I like this approach. It's the right way to deal with the race between our surface getting changed and gecko starting. However, it doesn't make sense to queue all events. If the user is on a slow device and starts doing random stuff to our app during early loading, we don't really want those events being handled. Also, we should only care about the last size event unless fennec is depending on multiple resize events, so we don't need a list - just the last resize event.

Eliminating setInitialSize is good - you should also kill the wrapper for setInitialSize in other-licenses/android/APKOpen.cpp.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #7)
> Comment on attachment 485307 [details] [diff] [review]
> patch
> 
> I like this approach. It's the right way to deal with the race between our
> surface getting changed and gecko starting. However, it doesn't make sense to
> queue all events. If the user is on a slow device and starts doing random stuff
> to our app during early loading, we don't really want those events being
> handled. Also, we should only care about the last size event unless fennec is
> depending on multiple resize events, so we don't need a list - just the last
> resize event.
> 
> Eliminating setInitialSize is good - you should also kill the wrapper for
> setInitialSize in other-licenses/android/APKOpen.cpp.

I'm not sure why we would want to filter events on the java side of things. The platform should coalesce events where its appropriate. IMO we should send all events and let the platform handle it.

Comment 9

[Michael Wu [:mwu]]

(In reply to comment #8)
> I'm not sure why we would want to filter events on the java side of things. The
> platform should coalesce events where its appropriate. IMO we should send all
> events and let the platform handle it.

It's not coalescing. It's just firing an move event when we gecko is ready to receive it. We don't care about any other events before gecko starts, so there's no point in queuing them.

Comment 10

[Michael Wu [:mwu]]

(In reply to comment #9)
> It's not coalescing. It's just firing an move event when we gecko is ready to
> receive it. We don't care about any other events before gecko starts, so
> there's no point in queuing them.

And by move I mean resize.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 12

[Michael Wu [:mwu]]

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

Please remove the setInitialSize wrapper in APKOpen.cpp http://hg.mozilla.org/mozilla-central/file/392605694d4b/other-licenses/android/APKOpen.cpp#l220 . r=me with that.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/b77a54634c1b

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101029 Firefox/4.0b8pre Fennec/4.0b2pre

Ran/tested : adb shell am start -a org.mozilla.gecko.DEBUG -n org.mozilla.fennec/org.mozilla.fennec.App

tried changing landscape/portrait w/ and w/o hardware keyboard.