1045824 - [B2G][Contacts][Facebook] Facebook log in screen is not rendered properly on screen when importing contacts.

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Martin Shuman [:Marty]]

Attachment: logcat-Contacts.txt

Description:
If the user tries to import their Facebook contacts, they are brought to a log in screen which does not render properly, blocking them from logging in.  This occurs both in the FTE and within the Contacts app.


Repro Steps:
1) Update a Flame to 20140729040211
2) Open the Contacts app and press the Gear icon to go into Settings
3) Select the Facebook Sync Friends toggle.
4) Observe the log in screen


Actual:
Log in screen is rendered improperly and is not usable.


Expected:
Log in screen is rendered properly

Environmental Variables:
Device: Flame Master
Build ID: 20140729040211
Gaia: fadfafa17f5175203b8b9457bfb95e5816f54f58
Gecko: b17cad2d1e5e
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Keywords: Facebook, Contacts, Import, FTU, FTE, Render

Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/5857/
See attached: screenshot, logcat

Comment 1

[Martin Shuman [:Marty]]

Attachment: Facebook_Import_Screenshot.png

Comment 2

[Martin Shuman [:Marty]]

I discovered a work-around for this issue.  If the user holds the Home button to go to the Task Manager (Card View), and then re-enters the Contacts app, the log-in screen will then be rendered properly.

Comment 3

[Martin Shuman [:Marty]]

This issue does NOT occur in Flame 2.0.
The Facebook log in screen is rendered properly.

Environmental Variables:
Device: Flame 2.0
Build ID: 20140729000201
Gaia: b11775fcbfe076a3fc560c2041f5b2fe1b345009
Gecko: 86b56e101512
Version: 32.0 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 4

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Regression, smoke test blocker.

Requesting a window.

Comment 5

[Joshua Mitchell (Inactive)]

B2G Inbound Regression Window:

Last Working:
Device: Flame Master
Build ID: 20140728052714
Gaia: d9afe81149d83ce34fbb68769c5820415580f6f7
Gecko: 0e48db820d99
Version: 34.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First Broken:
Device: Flame Master
Build ID: 20140728054813
Gaia: 3357c45e793bfca7648d215f378eb47e4e97aa23
Gecko: 0564c0d5a1bb
Version: 34.0a1 (Master)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Last Working Gaia First Broken Gecko: Issue does NOT reproduce
Gaia: d9afe81149d83ce34fbb68769c5820415580f6f7
Gecko:0564c0d5a1bb

First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: 3357c45e793bfca7648d215f378eb47e4e97aa23
Gecko:0e48db820d99

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare


From the pushlog it appears bug 1042744  could be the culprit - Vivien can you take a look?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Joshua Mitchell [:Joshua_M] from comment #5)
> B2G Inbound Regression Window:
> 
> Last Working:
> Device: Flame Master
> Build ID: 20140728052714
> Gaia: d9afe81149d83ce34fbb68769c5820415580f6f7
> Gecko: 0e48db820d99
> Version: 34.0a1 (Master)
> Firmware Version: v122
> User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
> 
> First Broken:
> Device: Flame Master
> Build ID: 20140728054813
> Gaia: 3357c45e793bfca7648d215f378eb47e4e97aa23
> Gecko: 0564c0d5a1bb
> Version: 34.0a1 (Master)
> Firmware Version: v122
> User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
> 
> Last Working Gaia First Broken Gecko: Issue does NOT reproduce
> Gaia: d9afe81149d83ce34fbb68769c5820415580f6f7
> Gecko:0564c0d5a1bb
> 
> First Broken Gaia Last Working Gecko: Issue DOES reproduce
> Gaia: 3357c45e793bfca7648d215f378eb47e4e97aa23
> Gecko:0e48db820d99
> 
> Gaia pushlog:
> https://github.com/mozilla-b2g/gaia/compare
> 
> 
> From the pushlog it appears bug 1042744  could be the culprit - Vivien can
> you take a look?

Chris, this sounds like a tiling issue. Do you have any ideas if scrollgrab can cause such behaviors ?

Comment 7

[Chris Lord [:cwiiis]]

Looks like a composition bounds calculation/progressive rendering bug. Changing component and cc'ing gfx peeps.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I'd like to wait until the regression from bug 1022612 are fixed before investigating.

Comment 9

[Joshua Mitchell (Inactive)]

Comment 5 was missing the complete pushlog link: here it is - https://github.com/mozilla-b2g/gaia/compare/d9afe81149d83ce34fbb68769c5820415580f6f7...3357c45e793bfca7648d215f378eb47e4e97aa23

Comment 10

[Tony Chung [:tchung]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> I'd like to wait until the regression from bug 1022612 are fixed before
> investigating.

Kats, what regression bug are you waiting on that was dependent on 1026612?   I talked to fabrice and its recommended in order to unblock QA, we focus on getting this bug fixed asap.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Bug 1041200 was a big one (landed), but not the only one.  I believe bug 1042772 could be part of the problem as well.  Either way, it'd be the same people looking at the fallout from bug 1022612, and those bugs are also 2.1? or 2.1+.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

As per the email thread I'll take ownership of this. Once but 1042772 is resolved I can look into this.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I'm able to reproduce this pretty easily even with the patches from bug 1042772 applied, so I'm looking into it. The first thing I noticed is that after that facebook login dialog pops up the ContainerLayer there has an empty composition bounds, which is coming from empty widget bounds at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=931943302ba5#781.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - (cleanup) Use nsIntRect instead of nsRect for things not in app units

nsRect holds app units, and the window size is not in app units. Also TabChild::mOuterRect was already a nsIntRect.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 (fix) - Set some dimensions on the dialog window being created

When content does a window.open call it creates a new TabChild. However that TabChild doesn't have any dimensions set which seems kind of bad. Here I assign the new TabChild the same dimensions/properties as the one it was spawned from. This fixes the problem.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Marking as blocks 1042744 since that was the change that revealed this bug (as per comment 5).

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Try push at https://tbpl.mozilla.org/?tree=Try&rev=c090fcbdd201 includes these patches.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

(various nsRect, nsIntRect etc really need better names to hint what coordinates they contain)

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8469518 [details] [diff] [review]
Part 2 (fix) - Set some dimensions on the dialog window being created

This looks wrong to me.
Parent should tell the size.

Looks like we end up to
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#2515
and then to http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1007
I think, so we just send Show(0, 0)
and then in http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1719 we don't even use that 0,0 for anything.

But I don't understand why http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1915 wouldn't be called

Comment 21

[Kartikaya Gupta (email:[e-mail])]

(In reply to Olli Pettay [:smaug] from comment #20)
> But I don't understand why
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.
> cpp#1915 wouldn't be called

I see this get called a bunch of times in the root process. It always seems to get called on the TabParent that already exists, and so the code at http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#572 prevents the message from getting sent over IPC.

I don't really know the frameloader code, and it's hard to debug things that don't get called without understanding where it's supposed to called from, because I can't just stick a gdb breakpoint. If you think the bug is somewhere in the frameloader code can you suggest somebody who can look into this issue?

Also I don't know if it helps to look at the patch on bug 1042744 that exposed this bug.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I think it is currently possible that when nsFrameLoader::SetRemoteBrowser is called
in http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLFrameElement.cpp#60 ,  which is called from
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp#227
we have already reflowed the created <iframe> and that is why we don't get the necessary
nsFrameLoader::UpdatePositionAndSize call.
So, perhaps one way to fix this is to call
UpdatePositionAndSize manually in 
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp#227 in
case popupFrameElement->GetPrimaryFrame() returns non-null.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - (fix) Ensure set position/size on the child

Your analysis was spot on, thanks! I put the call inside the CreateRemoteFrameLoader function instead because (a) I didn't want to call nsSubDocumentFrame::ReflowFinished() directly and (b) OpenWindowOOP doesn't seem to have a handle to the frameloader, which is needed to call UpdatePositionAndSize.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8470117 [details] [diff] [review]
Part 2 - (fix) Ensure set position/size on the child

I guess you could just do 
if (nsSubDocumentFrame* subdocFrame = do_QueryFrame(GetPrimaryFrame())) {, 
since do_QueryFrame is null-safe

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Updated as per comment 24, and also slightly reworded the comment so it was more clear. Landed:

https://hg.mozilla.org/integration/b2g-inbound/rev/90005bd54a62
https://hg.mozilla.org/integration/b2g-inbound/rev/d57900610be0

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90005bd54a62
https://hg.mozilla.org/mozilla-central/rev/d57900610be0

Comment 27

[Josh Schmitt [Joshs]]

Issue no longer repros on 2.1 Flame.

The page appears correctly.

Environmental Variables:
Device: Flame Master 319MB
Build ID: 20140811040202
Gaia: 19ed3c9e78eaf234cc08484bde6998ae21552ba5
Gecko: a9b43778f0c2
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 28

[bhavana bajaj [:bajaj]]

Switching the 2.1?->2.1+, on these fixed bugs as these are regression.

Nothing to land here, its just flag-cleanup of 2.1? list. Please Ni me if there is confusion/disagreement.