917310 - Apps install path is broken on b2g desktop for windows and mac

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[Alexandre Poirot [:ochameau]]

Gaia doesn't launch on b2g desktop for mac and windows due to a regression introduced in bug 905881. We should move the #ifdef B2G first here:
https://hg.mozilla.org/mozilla-central/rev/e6a9da6e6a8c#l6.117

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Fix app install path for b2g desktop on Windows and Mac r=fabrice

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 806008 [details] [diff] [review]
Fix app install path for b2g desktop on Windows and Mac r=fabrice

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

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 806008 [details] [diff] [review]
Fix app install path for b2g desktop on Windows and Mac r=fabrice

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

Actually, I think we need to make it even clearer what depends on the application and what depends on the platform.

So we could have:

#ifdef MOZ_B2G       // All b2g builds

#ifdef MOZ_WIDGET_GONK
#endif

#elifdef MOZ_FENNEC  // All fennec
#elifdef MOZ_PHOENIX // Firefox 

#ifdef XP_WIN
#elifdef XP_UNIX
#elifdef XP_MACOSX
#endif


#elifdef MOZ_WEBAPP_RUNTIME // You guessed it...
#else    // Anything unsupported, like Metro
ASSERT('!BOOM!')
#endif

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Actually, I think we need to make it even clearer what depends on the
> application and what depends on the platform.

I think we should just have a definition for each platform, like:
#ifdef MOZ_B2G

#elifdef MOZ_FENNEC

#elifdef XP_WIN

#elifdef XP_MACOSX

#elifdef XP_UNIX

#endif

After bug 910473 we'll have the B2G and Fennec part in their own directory.

We shouldn't have a different code path for the Webapp Runtime and Firefox.

Comment 5

[[:fabrice] Fabrice Desré]

B2G is not a platform, it's a product, that runs on different platforms. Same for firefox desktop and the webapprt.

Comment 6

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> B2G is not a platform, it's a product, that runs on different platforms.
> Same for firefox desktop and the webapprt.

B2G has always the same installation path on every platform, so there's no need to have
#ifdef MOZ_WIDGET_GONK
#endif

Firefox desktop and the Webapp Runtime also have the same installation directory, there's no need to have different code paths for them.

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #6)
> (In reply to Fabrice Desré [:fabrice] from comment #5)
> > B2G is not a platform, it's a product, that runs on different platforms.
> > Same for firefox desktop and the webapprt.
> 
> B2G has always the same installation path on every platform, so there's no
> need to have
> #ifdef MOZ_WIDGET_GONK
> #endif
> 
> Firefox desktop and the Webapp Runtime also have the same installation
> directory, there's no need to have different code paths for them.

You're missing the point here. I want the top level #ifdef to be products, not platforms. If a product behaves the same on all platforms, fine, no other nested #ifdefs. But keeping #ifdef MOZ_B2G at the same level as #ifdef XP_WIN is just wrong.

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> You're missing the point here. I want the top level #ifdef to be products,
> not platforms. If a product behaves the same on all platforms, fine, no
> other nested #ifdefs. But keeping #ifdef MOZ_B2G at the same level as #ifdef
> XP_WIN is just wrong.

Oh, ok. I agree.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Another ifdef layout

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 806082 [details] [diff] [review]
Another ifdef layout

It could have been simplier as webapps runtime and firefox are the same today, but #if defined(xxx) || defined(yyy) doesn't seem to work in js files.

Comment 11

[Marco Castelluccio [:marco]]

I'd prefer the other ifdef layout with an explanatory comment:
#ifdef MOZ_B2G

#elifdef MOZ_FENNEC

// Firefox and Webapp Runtime share the same installation path format
#elifdef XP_WIN

#elifdef XP_MACOSX

#elifdef XP_UNIX

Comment 12

[Marco Castelluccio [:marco]]

It's up to Fabrice to decide the ifdef layout, but anyway the patch won't work on Mac.
On Mac both XP_MACOSX and XP_UNIX are defined, so you should put XP_MACOSX before XP_UNIX.

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 806350 [details] [diff] [review]
Patch

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

r=me
Let's do that for now, and clean this up in Bug 910473

Comment 16

[[:fabrice] Fabrice Desré]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Could/should we have tests that would catch this in the future? This seems like a pretty bad regression to not catch on TBPL when bug 905881 landed.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fc5d65192ae

Comment 19

[Bob Silverberg [:bsilverberg]]

I think the issue is that this didn't manifest on Linux, which is why TBPL didn't catch it. Is it possible to have a version of the gaia-ui-tests job on TBPL run on OS X?

Comment 20

[Marco Castelluccio [:marco]]

(In reply to Bob Silverberg [:bsilverberg] from comment #19)
> I think the issue is that this didn't manifest on Linux, which is why TBPL
> didn't catch it. Is it possible to have a version of the gaia-ui-tests job
> on TBPL run on OS X?

This is also why I didn't catch the problem in my local testing. I tested the patch in Mac, Windows, Linux, Android, B2G on Linux, but I didn't test in B2G on other platforms.

I'm really sorry for this!

Comment 22

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #18)
> https://hg.mozilla.org/mozilla-central/rev/9fc5d65192ae

Seems like this patch broke the homescreen for me on the device. I can see this error with logcat:

E/GeckoConsole(  689): [JavaScript Error: "[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXULAppInfo.ID]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/WebappOSUtils.jsm :: <TOP_LEVEL> :: line 83"  data: no]" {file: "resource://gre/modules/WebappOSUtils.jsm" line: 83}]

I wonder if this is related to the fact that the homescreen is runned into a child process.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Patch with good old ifdefs, hotfix on top of master

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: Fixed ifdefs

This patch actually work.

Verified by running gaia on device,
and running dom/apps,toolkit/devtools/apps tests on firefox.

I haven't been able to run webappsrt tests,
and I don't have a b2g desktop build ready.

Comment 25

[Marco Castelluccio [:marco]]

MOZ_PHOENIX is defined for the Webapp Runtime, so the |#elifdef MOZ_WEBAPP_RUNTIME| isn't needed.

Comment 26

[Marco Castelluccio [:marco]]

MOZ_PHOENIX is also defined for Metro, so we wouldn't throw on Metro.

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #25)
> MOZ_PHOENIX is defined for the Webapp Runtime, so the |#elifdef
> MOZ_WEBAPP_RUNTIME| isn't needed.

Perfect is the enemy of good.

Comment 28

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #26)
> MOZ_PHOENIX is also defined for Metro, so we wouldn't throw on Metro.

About this, you could do:
#elifdef MOZ_METRO
...
#elifdef MOZ_PHOENIX
...
#endif

I agree the other problem is a nit that you could avoid fixing.

Comment 29

[[:fabrice] Fabrice Desré]

I tested on device, desktop-linux and deskop-mac with Alex's patch. They all ran fine. We need to unblock b2g so I'm just gonna land that.

Comment 30

[Marco Castelluccio [:marco]]

Attachment: Patch

This is exactly like ochameau's patch, the only difference is #ifdef MOZ_METRO to exclude Metro.

Could someone test this on device? Testing the patch on b2g desktop and on the try server isn't enough, I didn't know that and I'm really sorry for the breakage.

Comment 31

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #29)
> I tested on device, desktop-linux and deskop-mac with Alex's patch. They all
> ran fine. We need to unblock b2g so I'm just gonna land that.

I'm ok with this.

Comment 32

[[:fabrice] Fabrice Desré]

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

Comment 33

[Ed Morley [:emorley]]

Double landed on m-c, so we can get this out faster:
https://hg.mozilla.org/mozilla-central/rev/e8bb95435a54

Comment 34

[Jason Smith [:jsmith]]

Fixed by comment 33

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6933a2fda563

Comment 36

[Alexandre Poirot [:ochameau]]

Comment on attachment 806719 [details] [diff] [review]
Fixed ifdefs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 905881 reached ff26 branch, whereas this regression fix didn't. Nor the other regression in bug 918611.
User impact if declined: 
Testing completed (on m-c, etc.): baked on m-c and regression have been fixed.
Risk to taking this patch (and alternatives if risky): gaia won't start on b2g desktop 1.2.
String or IDL/UUID changes made by this patch: none

If we approve uplift here, we should uplift the two revisions being pushed in this bug:
  https://hg.mozilla.org/mozilla-central/rev/9fc5d65192ae
  https://hg.mozilla.org/mozilla-central/rev/6933a2fda563
and also the changeset from bug 918611:
  https://hg.mozilla.org/mozilla-central/rev/d5de24a40c49

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Pushed as a single roll-up patch.

https://hg.mozilla.org/releases/mozilla-aurora/rev/a98fdbb5d68d