959897 - [b2g][system][v1.2] all apps get mozapptype set to 'critical'

Status: RESOLVED WONTFIX

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

Description

[Ben Kelly [:bkelly, not reviewing]]

While investigating bug 957346 I discovered that v1.2 has a bug which results in the mozapptype being incorrectly set to 'critical' for all apps.  This is caused by by this code:

  https://github.com/mozilla-b2g/gaia/blob/v1.2/apps/system/js/browser_frame.js#L69

Specifically, the following function call will always return true:

      config.url.startsWith(window.location.protocol +
                              '//communications.gaiamobile.org' +
                              window.location.port ?
                              '' : (':' + window.location.port) +
                              '/dialer')

This is due to order of operations on the inline conditional.  The intention was for |window.location.port| to be used in the conditional statement to control if the port is appended or not.  Unfortunately, though, the string concatenation binds more tightly and the condition triggers on the entire |window.location.protocol + '//communications.gaiamobile.org' + window.location.port|.  This of course is truthy which results in |config.url.startsWith('')| being executed.  This will always return true.

This bug was fixed as a side effect of bug 905116 with this commit:

  https://github.com/mozilla-b2g/gaia/commit/afd555ae68dd12429bd9162d7826b8329f251f90

I'm writing this bug in case we want to fix this on v1.2 before its frozen.  Also, it may be useful information when diagnosing differences between v1.2 and v1.3.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

My personal opinion is that it would be dangerous to try fixing this in v1.2 now.  It would potentially change how OOM killing performs which in turn might suddenly make a lot of test cases behave differently.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Alive, what do you think?  Should we try to fix the setting of mozapptype in v1.2?

Comment 3

[Jason Smith [:jsmith]]

I don't think we can fix this at this point - we're done with 1.2 & are not planning to ship this branch to any operator.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

What Jason said. :(

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

The side effect of this bug is the process priority would be the same -- as callscreen.

Comment 6

[Jason Smith [:jsmith]]

Moving to WONT FIX.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> The side effect of this bug is the process priority would be the same -- as
> callscreen.

I think we just need to keep this in mind when debugging background OOM kill "regressions" compared to v1.2.