Closed Bug 1045824 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
blocking-b2g 2.1+
Tracking Status
b2g-v2.0 --- unaffected
b2g-v2.1 --- verified

People

(Reporter: Marty, Assigned: kats)

References

Details

(Keywords: regression, smoketest)

Attachments

(4 files, 1 obsolete file)

Attached file 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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
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.
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
[Blocking Requested - why for this release]:
Regression, smoke test blocker.

Requesting a window.
blocking-b2g: --- → 2.1?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
QA Contact: jmitchell
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?
Flags: needinfo?(21)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
(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 ?
Flags: needinfo?(21) → needinfo?(chrislord.net)
Looks like a composition bounds calculation/progressive rendering bug. Changing component and cc'ing gfx peeps.
Component: Gaia::Contacts → Graphics: Layers
Flags: needinfo?(chrislord.net)
Product: Firefox OS → Core
Version: unspecified → Trunk
I'd like to wait until the regression from bug 1022612 are fixed before investigating.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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+.
As per the email thread I'll take ownership of this. Once but 1042772 is resolved I can look into this.
Assignee: nobody → bugmail.mozilla
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
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.
Component: Graphics: Layers → DOM: Content Processes
nsRect holds app units, and the window size is not in app units. Also TabChild::mOuterRect was already a nsIntRect.
Attachment #8469514 - Flags: review?(bugs)
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.
Attachment #8469518 - Flags: review?(bugs)
Marking as blocks 1042744 since that was the change that revealed this bug (as per comment 5).
Blocks: 1042744
Attachment #8469514 - Flags: review?(bugs) → review+
(various nsRect, nsIntRect etc really need better names to hint what coordinates they contain)
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
Attachment #8469518 - Flags: review?(bugs) → review-
(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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
Attachment #8469518 - Attachment is obsolete: true
Attachment #8470117 - Flags: review?(bugs)
Attachment #8470117 - Flags: review?(bugs) → review+
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
https://hg.mozilla.org/mozilla-central/rev/90005bd54a62
https://hg.mozilla.org/mozilla-central/rev/d57900610be0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+][lead-review+] → [QAnalyst-Triage?][lead-review+]
Flags: needinfo?(pbylenga)
QA Whiteboard: [QAnalyst-Triage?][lead-review+] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(pbylenga)
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.
blocking-b2g: 2.1? → 2.1+
You need to log in before you can comment on or make changes to this bug.