959800 - [Offline] mozbrowser iframes don't display correct offline content

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Josh Schmitt [Joshs]]

Attachment: Close_button.png

Description:
The user encountered an error message that the user could not close when selecting the close button while trying to import a Google account.

Repro Steps:
1) Updated Buri to BuildID: 20140113004002
2) Proceed to setting and turn off wifi and data
3) Proceed to the Calendar app
4) Select the 'Drawer' icon
5) Select the '+' icon
6) Select 'Google'

Actual:
Selecting the 'Cancel' button does not cancel the current action or close the error message.

Expected:
Selecting the 'Cancel' button will take the user back to the account page and close the error message.

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140113004002
Gaia: b3fc4f712562ee92b0ed0bd17abc61be9a36a8da
Gecko: 5bb1837de7c0
Version: 28.0a2
Firmware Version: V1.2-device.cfg

Comment 1

[Josh Schmitt [Joshs]]

Does not reproduce in the calendar app on 1.2

Environmental Variables:
Device: Buri 1.2 COM
BuildID: 20140113004002
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: e672faf1e6a1
Version: 26.0
RIL Version: 01.02.00.019.102
Firmware Version: V1.2-device.cfg

Comment 2

[Jason Smith [:jsmith]]

This likely implies that the patch in bug 942325 didn't fix the bug correctly, as this means we're still showing the close button on a mozbrowser iframe.

Comment 3

[Preeti Raghunath(:Preeti)]

Gregor,

Please investigate for sfe.

Comment 4

[Gregor Wagner [:gwagner]]

Mike, can you investigate?

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

Yup, bug 942325 didn't fix the mozbrowser iframe case, since we use mozbrowser for apps too. I didn't know we use mozbrowser frames within apps. I'm wondering if we can get away with not using mozbrowser iframe here: 

https://github.com/mozilla-b2g/gaia/blob/4416166f0d228cdd727d1a425900dc9177088314/apps/calendar/js/oauth_window.js#L139


In any case, I will take this.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Michael Henretty [:mhenretty] from comment #5)
> Yup, bug 942325 didn't fix the mozbrowser iframe case, since we use
> mozbrowser for apps too. I didn't know we use mozbrowser frames within apps.
> I'm wondering if we can get away with not using mozbrowser iframe here: 
> 
> https://github.com/mozilla-b2g/gaia/blob/
> 4416166f0d228cdd727d1a425900dc9177088314/apps/calendar/js/oauth_window.
> js#L139
> 
> 
> In any case, I will take this.

mozbrowser is allowed to be used by third party privileged apps, so I don't think we can solve this by removing the mozbrowser iframe from the calendar app. We really need to fix this generally for all mozbrowser iframes.

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

I spoke with Fabrice about this on IRC. Currently there is no way to tell the difference between being in an app vs being in a mozbrowser iframe. It seems we will need to pass a flag from gecko "isApp" or something to detect the difference from net_error.

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Michael Henretty [:mhenretty] from comment #7)
> Currently there is no way to tell
> the difference between being in an app vs being in a mozbrowser iframe.

This is not the exact problem. The real problem is that, according to spec, browser tabs need to show both "Retry" and "Close" buttons, whereas iframes need to have only a "Retry" button. Right now, there is no way to distinguish between a browser tab, and a mozbrowser iframe within an app from net_error.html. I think the best way forward is to embed the browser manifest url into net_error.html at build time, so that we can check if we are in a browser tab at runtime.

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Append frameType to about:error URL

https://tbpl.mozilla.org/?tree=Try&rev=c5f776e441fb

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Use frameType, and browser manifest url to determine if we are framed

Note: this will not pass in Travis until we land the above gecko patch.

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 8366307 [details] [diff] [review]
Append frameType to about:error URL

Review of attachment 8366307 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really not a fan of using the int value there. You should map it to the "regular", "browser", "app" string instead.

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: frametype.patch

https://hg.mozilla.org/try/rev/bfc570574ec1

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8366321 [details] [review]
Use frameType, and browser manifest url to determine if we are framed

Updated the pull request to use descriptive frameType strings.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8367019 [details] [diff] [review]
frametype.patch

Review of attachment 8367019 [details] [diff] [review]:
-----------------------------------------------------------------

Please ask review to a docShell peer, like bz.

::: docshell/base/nsDocShell.cpp
@@ +4776,5 @@
>        errorPageUrl.AppendLiteral("&m=");
>        errorPageUrl.AppendASCII(manifestParam.get());
>      }
>  
> +    nsCString frameType = NS_ConvertUTF16toUTF8(FrameTypeToString(mFrameType));

s/nsCString/nsAutoCString

::: docshell/base/nsDocShell.h
@@ +666,5 @@
>          eFrameTypeBrowser,
>          eFrameTypeApp
>      };
>  
> +    static const nsString FrameTypeToString(FrameType aFrameType)

you could use nsAutoCString and NS_LITERAL_CSTRING since this is what you actually want.

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Add frameType to about:*error documentURI

Hi BZ, could you review this change to DocShell?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8368233 [details] [diff] [review]
Add frameType to about:*error documentURI

sr=me

Comment 17

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c427ea289e1b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c427ea289e1b

Comment 19

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/f1928155eb29b6b1766ba6ca8380221a4f670d60

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ae9d67a8df5

Looks like the Gaia patch has conflicts with the v1.3 branch. I'm going to have to leave that for someone else to cherry-pick.

Comment 21

[Jason Smith [:jsmith]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #20)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/2ae9d67a8df5
> 
> Looks like the Gaia patch has conflicts with the v1.3 branch. I'm going to
> have to leave that for someone else to cherry-pick.

Mike - Can you cherry pick this?

Comment 22

[Michael Henretty [:mikehenrty][:mhenretty]]

Submitting a new pull request for the uplift to make sure this doesn't burn aurora travis.

https://travis-ci.org/mozilla-b2g/gaia/builds/18114735

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: 1.3 Specific Uplift

Needed to change some existing 1.3 specific code, so having aus take a look.

Comment 24

[Aus Lacroix [:aus]]

Comment on attachment 8369922 [details] [review]
1.3 Specific Uplift

Looks good! I like the addition of caching the error as well. When we have travis green, let's land it!

Comment 25

[Michael Henretty [:mikehenrty][:mhenretty]]

uplifted

v1.3: https://github.com/mozilla-b2g/gaia/commit/0388dcb7621c5933712680a258ce94ead389e2b6

Comment 26

[Jason Smith [:jsmith]]

Verified on trunk using comment 0's STR - lgtm.