1082139 - JavascriptException: JavascriptException: TypeError: window.getComputedStyle(...) is null at: app://callscreen.gaiamobile.org/gaia_build_defer_index.js line: 146

Status: RESOLVED FIXED

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

Description

[Anshul]

While running various call related tests I am seeing the following error

JavascriptException: JavascriptException: TypeError: window.getComputedStyle(...) is null at: app://callscreen.gaiamobile.org/gaia_build_defer_index.js line: 146

[Blocking Requested - why for this release]: This is preventing us to do run unit tests for voice calls to ensure the quality of this release.

Comment 1

[Kevin Grandon :kgrandon]

Seems like this is a dialer bug?

Comment 2

[Kevin Grandon :kgrandon]

I *think* that this line, before optimizing, points to here: https://github.com/mozilla-b2g/gaia/blob/a886862ac0f74c01a832e57a986095507ee3b51c/shared/js/dialer/font_size_manager.js#L71

This line of code was added here: https://github.com/mozilla-b2g/gaia/commit/715f3e9d163c87baea76021f6873a2eeb018c8ff#diff-bc0feadf5c699af9ea9684b6b18be649R71

Adding some people to the CC who have worked on this file.

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: blocking testing

Comment 4

[Germán Toro del Valle (:gtorodelvalle)]

Hi Anshul, could you provide further details about the tests you are running? I checked the code and it seems legit, at least from a Dialer and related apps perspective.

On the other hand and although it is almost ancient history, could the issue be related to bug 548397? In fact that's the reason why I am asking for more information about the tests ran :)

Thanks!

Comment 5

[Anshul]

I couldn't pin point to exact step that is causing this issue so I am listing what all I am doing in my test script.

1. Make an outgoing call, connect the call and after 2 secs disconnect the call
2. Make an incoming call, answer the call add after 2 secs disconnect the call
3. Make an outgoing call, connect the call, make an incoming call when previous is still connected, answer the incoming call, after 2 secs disconnect the incoming call, after 2 secs disconnect the outgoing call.

Comment 6

[Doug Sherk (:drs) (inactive)]

If we can repro the problem, a good way to verify the hypothesis in comment 4 could be something like this, instead of calling `window.getComputedStyle()` directly: http://jsfiddle.net/d4hkbq2m/

Comment 7

[Germán Toro del Valle (:gtorodelvalle)]

Hi Doug, I think the issue is a little more complex than what you comment in comment 6. As far as my understanding, the problem is not related to the element whose computed style wants to be obtained having |display: none| but with the iFrame, in whose document the element is, having |display: none|.

This codepen reproduces the issue: http://codepen.io/gtorodelvalle/pen/wLHgi (as you'll see it includes an iFrame whose source is basically this: http://codepen.io/gtorodelvalle/full/seCKA (you can check the source here: http://codepen.io/gtorodelvalle/pen/seCKA ). If you set the display property of the iFrame (not the paragraph element) to 'none' you'll see 'null' on the console, whereas if you set any other display value it will log the computed style. The visibility of the paragraph has no influence on the result.

SIDE NOTE: To force the reloading of the 'main' codepen after changing the display property value just cut (change) the src attribute value of the iFrame, wait a couple of seconds until the codepen result view is reloaded and paste it back again.

So the point here is how to get the visibility status of the iFrame 'hosting' the call screen app to dismiss or to confirm the hypothesis. Any clues?

Anshul, would it be possible to have a look at the source code of the test or battery of tests which are causing the issue? Thanks!

I am currently discussing the issue internally with our QA team.

Comment 8

[Germán Toro del Valle (:gtorodelvalle)]

BTW, I saw this bug was set as 2.1+ but I am not really sure which version you are using in your tests, Anshul. I am testing and working with 2.2. Could you please clarify this, please? ;) Thanks! Just in case the tag is not right.

Comment 9

[Anshul]

Germán, the issue is reproducible on 2.1. The tests by themselves wouldn't be very useful to you because you won't be able to run them as it includes a lot of proprietary glue. As I mentioned in comment #5 the series of steps that the test script takes.

Comment 10

[Doug Sherk (:drs) (inactive)]

The Callscreen itself is an iframe within the System app. My guess is that we're calling `window.getComputedStyle()` before the Callscreen is fully loaded, or perhaps while it's being destroyed. It's not clear to me why this is happening, especially given the 2s delays listed in comment 5. But we don't have a good STR to investigate further with.

We can get around this fairly easily by just bailing out of `adaptToSpace()` if the `window.getComputedStyle()` call returns `null`. I can't think of any case where we would need `adaptToSpace()` to run to completion anyways when this happens.

Comment 11

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Pull request 25322

Hi Anshul, I checked that https://github.com/gtorodelvalle/gaia/commit/fe6361c9b535cff42e2f42bc123854739a0b9455.patch can be applied to v2.1 without errors. Would you be so kind to apply this patch to v2.1 and run the tests again checking if the console errors disappear? Thanks!

Comment 12

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8507814 [details] [review]
Pull request 25322

A small one, but a good starter.

Comment 13

[Anshul]

(In reply to Germán Toro del Valle from comment #11)
> Created attachment 8507814 [details] [review]
> Pull request 25322
> 
> Hi Anshul, I checked that
> https://github.com/gtorodelvalle/gaia/commit/
> fe6361c9b535cff42e2f42bc123854739a0b9455.patch can be applied to v2.1
> without errors. Would you be so kind to apply this patch to v2.1 and run the
> tests again checking if the console errors disappear? Thanks!
I am not able to reproduce the issue with this patch.

Comment 14

[Tamara Hills [:thills]]

Comment on attachment 8507814 [details] [review]
Pull request 25322

Hi German,

It looks good, but just wondering if we can explore adding a test.  I took a bit of a look and looks like maybe we could try testing it from keypad_test.js?

I had one small nit in the PR as well.

Thanks,

-tamara

Comment 15

[Doug Sherk (:drs) (inactive)]

(In reply to Tamara Hills [:thills] from comment #14)
> I took a bit of a look and looks like maybe we could try testing it from
> keypad_test.js?

There are tests for the affected file here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/sharedtest/test/unit/dialer/font_size_manager_test.js

Comment 16

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8507814 [details] [review]
Pull request 25322

Hi Tamara :) Patch updated solving the indentation issue and including the requested test cases ;) Thanks for your comments!

Comment 17

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8507814 [details] [review]
Pull request 25322

See my comments on the PR. Please re-request review from Tamara and then feedback from me once everything has been dealt with.

Comment 18

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8507814 [details] [review]
Pull request 25322

Hi guys! Thanks for your comments ;)

Comment 19

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8507814 [details] [review]
Pull request 25322

Thanks, Germán. I left one last comment. Tamara, I trust you to take this to completion if you're not going to give it r+ already.

Comment 20

[Tamara Hills [:thills]]

Comment on attachment 8507814 [details] [review]
Pull request 25322

Hi German,

Looks good.  There are two minor comments in the PR.  I did test it out and used the STR and was unable to reproduce the error.

Thanks,

-tamara

Comment 21

[Germán Toro del Valle (:gtorodelvalle)]

Latest comments included into the final patch ;)

Waiting for the tests to pass before merging ;)

Thanks!

Comment 22

[Germán Toro del Valle (:gtorodelvalle)]

Merging since the issue in gaia-try is not related to the updates included in this patch ;)

Merged in master: https://github.com/mozilla-b2g/gaia/commit/ad1baacdd8e80a0f877ccf6f6e8727d1769645e6

Comment 23

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8507814 [details] [review]
Pull request 25322

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Impedes running unit tests for voice calls to ensure the quality of this release
[User impact] if declined: It is more a QA / testing issue
[Testing completed]: Verified by QA that the patch solves the issue
[Risk to taking this patch] (and alternatives if risky): Very low (it just includes safer (more controls) code ;)
[String changes made]: None

As additional info: I just confirmed that the patch can be uplifted to v2.1 without problems ;)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/fc85c28243d2d8baf90a76bbf36b8c1e608c190a

Comment 25

[Yeojin Chung [:YeojinC]]

Unable to verify as it is a back-end issue.