631049 - widget created for combobox in the child process in Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alon Zakai (:azakai)]

In Fennec, we should render comboboxes and so forth in frontend code in the parent process (this is different than how desktop Firefox does things). However, it turns out we create and reflow comboboxes in platform code in the child process, which is apparently wrong. This was noticed during work on bug 610670 - the combobox does a reflow, and ends up hiding the widget that we care about in that bug. So this is both wrong and harmful.

Comment 1

[Alon Zakai (:azakai)]

Steps to reproduce:

1. Debug the child process in Fennec.
2. Add a break in nsComboboxControlFrame (constructor, or ::Reflow, etc.)
3. Visit http://www.google.com/m/news?q=wiki

The break will be hit.

Asking for blocking, since (1) this blocks a blocker (although, there is a workaround), and (2) I am guessing that fixing this would save us some memory and some code execution.

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to comment #0)
> 610670 - the combobox does a reflow, and ends up hiding the widget that we care
> about in that bug. So this is both wrong and harmful.

It looks like the combobox dropdown gets a widget and that is the widget that is getting hidden, not the toplevel puppet widget.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The reflow is correct, fwiw.  It needs to happen to size the combobox correctly.

Comment 4

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

Is this the reason for Bug 630946?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #4)
> Is this the reason for Bug 630946?

No

Comment 6

[Timothy Nikkel (:tnikkel)]

I think that the widget is created in nsCSSFrameConstructor::InitializeSelectFrame.

Comment 7

[Alon Zakai (:azakai)]

1. This turns out to not be important for bug 610670, so it doesn't seem urgent to me anymore.

2. After more discussion with mfinkle and others, I am no longer sure if this is an actual bug. Should no popup widgets be created in the child process?

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to comment #7)
> 2. After more discussion with mfinkle and others, I am no longer sure if this
> is an actual bug. Should no popup widgets be created in the child process?

Does Fennec use the created widgets? If not then they shouldn't be created.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note that the fact that widget is eagerly created here is a bug in itself; we have that on file.  It shouldn't be created until we try to open the popup (which sounds like it might be "never" in the fennec case).

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> Note that the fact that widget is eagerly created here is a bug in itself; we
> have that on file.  It shouldn't be created until we try to open the popup
> (which sounds like it might be "never" in the fennec case).

Exactly

Comment 11

[Alon Zakai (:azakai)]

So, when we create a page with a combobox, we call nsCSSFrameConstructor::ConstructSelectFrame. We then call InitializeSelectFrame with aBuildCombobox set to True. This leads to creation of a new Puppet Widget through the view's CreateWidgetForPopup.

I don't see anything that looks odd, seems like creating a widget during page load here is normal. Should something else be happening - what are we trying to fix here?

Comment 12

[Timothy Nikkel (:tnikkel)]

That is all normal. Since in the Fennec case that widget is totally useless we should not create it. A related code cleanup task would be to only create the widget for the select on demand, ie when it needs to be shown. That would fix this bug as well.

Comment 13

[Boris Zbarsky [:bzbarsky]]

We have an existing bug on doing that last part; it would help desktop a quite a bit too.

Comment 14

[Alon Zakai (:azakai)]

Got it, thanks tn and bz.

bz, is the bug you meant bug 610391?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yes.

Comment 16

[Alon Zakai (:azakai)]

Attachment: patch

If we do it like this, we need to check with Camino, which also uses nsComboboxControlFrame::ToolkitHasNativePopup. This patch should help them as well, but need to verify it doesn't break things for them somehow.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

This patch breaks listboxes completely (in the "always render wrong, always leak, crash if the page tries" way) when !useNativePopup.

Which makes me suspect it has never been tested.

Comment 18

[Alon Zakai (:azakai)]

I tested it with the combobox in the web page in comment 1, and that worked. I will test on more stuff.

Comment 19

[Alon Zakai (:azakai)]

Attachment: patch v2

Sorry about the previous patch, I misunderstood something in the code, and didn't realize the patch affected listboxes as well (so I only tested on comboboxes). Here is a better version.

This patch works with comboboxes and listboxes, both rendering and functionality. Only thing I didn't fully check is leaking, since checking that in a Fennec build doesn't work (we always report leaks there) - but, with this patch we report slightly fewer leaks/bloat than before (of widgets, as expected).

Anything else I should check?

Comment 20

[Boris Zbarsky [:bzbarsky]]

That last patch looks good.  The only other thing to check is that nsComboboxControlFrame and nsListControlFrame don't assume a non-null widget.  Well, and that both listbox and combobox work when !useNativePopup.

Comment 21

[Alon Zakai (:azakai)]

cc'ing ardissone since this might affect Camino.

Comment 22

[Alon Zakai (:azakai)]

Comment on attachment 519498 [details] [diff] [review]
patch v2

This looks good on try for desktop, and for fennec I don't see any dangerous places in the code where a widget is assumed to exist (in fact, ToolkitHasNativePopup is used in the right places for that already), so asking for review.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 519498 [details] [diff] [review]
patch v2

This looks fine, but the patch in bug 610391 removes this code altogether.  Will ehsan's new code just not get hit in fennec, or does it need to be modified somehow?  Please talk to him.

Comment 24

[(no longer active)]

Hmm, which process is the ShowList call run in on Fennec?  If it's run in the content process, then I guess this is WFM based on the patch in bug 610391.

Comment 25

[Alon Zakai (:azakai)]

(In reply to comment #24)
> Hmm, which process is the ShowList call run in on Fennec?  If it's run in the
> content process, then I guess this is WFM based on the patch in bug 610391.

Yes, this stuff is done in the content process. And I checked now, it does indeed work as you said - no widget is created either on load or when it is clicked. Turns out this is because ShowList isn't called in Fennec at all (that stuff is done in Fennec frontend code).

So bug 610391 indeed fixes this one, hurray :)

Comment 26

[(no longer active)]

I love it when bugs fix themselves.  ;-)