839454 - [Facebook] webapps optimize minimizer and friends break Facebook integration

Status: RESOLVED WORKSFORME

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

Description

[Jose Manuel Cantera]

if web apps optimize is enabled in the makefile FB integration is broken. random errors happen when clicking on Update imported friends, such as [JavaScript Error: "TypeError: cancelButton is null" {file: "app://communications.gaiamobile.org/facebook/gaia_build_fb_oauth.js" line: 148}]

I'm gonna make a PR that puts communications in the black list for optimizing

Comment 1

[Jose Manuel Cantera]

Attachment: Pointer to GH PR 8025

Comment 2

[alberto.pastor]

Probably because Bug 835791 landed, but I can't see any error when removing Communications from the optimisations black list.
Can we re-test and enable it again?

Thanks!

Comment 3

[alberto.pastor]

Any update on this?

Comment 4

[alberto.pastor]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8025

Pointer to Github pull-request

Comment 5

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

This was caused by Bug 842249 after all.

Comment 6

[Jose Manuel Cantera]

landing this bug in v1.0.1 has broken Facebook import. Please do not uplift bugs without testing them.

Comment 7

[Jose Manuel Cantera]

(In reply to Jose M. Cantera from comment #6)
> landing this bug in v1.0.1 has broken Facebook import. Please do not uplift
> bugs without testing them.

I mean reverting the change that this bug introduced.

Comment 8

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Jose, is that landing 842249 that broke Facebook import ?
Could you help uplifting it then, as I don't know this code at all ? it looked like an easy one.

Comment 9

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Jose, is that landing 842249 that broke Facebook import ?
> Could you help uplifting it then, as I don't know this code at all ? it
> looked like an easy one.

The problem is that it is needed to have in the whitelist of build/webapps-optimizer.js 

const JS_AGGREGATION_BLACKLIST = [
  // https://bugzilla.mozilla.org/show_bug.cgi?id=839574
  'system',
  'communications'
];

otherwise Facebook import does not work

Comment 10

[Jose Manuel Cantera]

(In reply to Jose M. Cantera from comment #9)
> (In reply to Julien Wajsberg [:julienw] from comment #8)
> > Jose, is that landing 842249 that broke Facebook import ?
> > Could you help uplifting it then, as I don't know this code at all ? it
> > looked like an easy one.
> 
> The problem is that it is needed to have in the whitelist of
> build/webapps-optimizer.js 
> 
> const JS_AGGREGATION_BLACKLIST = [
>   // https://bugzilla.mozilla.org/show_bug.cgi?id=839574
>   'system',
>   'communications'
> ];
> 
> otherwise Facebook import does not work

 [JavaScript Error: "TypeError: messages[type] is null" {file: "app://communications.gaiamobile.org/facebook/gaia_build_fb_oauth.js" line: 147}]

Comment 11

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Jose, this is not like this on master; is it broken on master too ?

Maybe it's a problem similar to Bug 842249 that should be fixed too ?

Comment 12

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

This would be a shame to blacklist the whole communications (dialer, contacts...) because of this, I'd really want to fix this specific problem instead because I believe that not optimizing is just hiding the problem instead of fixing it.

But we can still blacklist communications if we find this happens in master too. If this is does not happen on master, then it means some other patches needs to be uplifted too.

Comment 13

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> This would be a shame to blacklist the whole communications (dialer,
> contacts...) because of this, I'd really want to fix this specific problem
> instead because I believe that not optimizing is just hiding the problem
> instead of fixing it.
> 
> But we can still blacklist communications if we find this happens in master
> too. If this is does not happen on master, then it means some other patches
> needs to be uplifted too.

in master is not happening

Comment 14

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

ok. I'm investigating then.

Comment 15

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I see this on v1-train when flashing with an unoptimized build so this is certainly not because of the optimization pass.

Comment 16

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Bug 844100 broke this by removing the "service" parameter in "OAuthFlow.start".

Jose, could you please fix this and land this on v1-train ? v1.0.1 seems fine.

Comment 17

[Jose Manuel Cantera]

what you have seen in v1-train is a different issue tracked in bug 851188 

and fixed by  

https://github.com/mozilla-b2g/gaia/pull/8667

Comment 18

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Except this pull request fixes the facebook integration.

So it really was not a different issue after all :)

Comment 19

[Jose Manuel Cantera]

but the issue in v1.0.1 has to do with the optimizer

Comment 20

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I'll check again in v1.0.1 but it seemed to work for me (?).

For you, does it fail as soon as you click on the "import" button, or should I try to actually import contacts ?

Comment 21

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

ok, to see it I have to actually go through all the process.
(why should the facebook app ask for my posts by the way ?)

Comment 22

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

fails also on v1-train when flashing with GAIA_OPTIMIZE=1
(now will try master)

Comment 23

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I'm happy to say that the same fails on master.

Comment 24

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I have a clue on what fails: because of the optimization, the JS in fb_oauth.html runs faster than before, and now it runs before the second frame is loaded. Therefore, the various bindings that it does at load (the various "document.getElementById") fail and get null instead of the elements.

I have some code to change that [1] and now it fails later with :

E/GeckoConsole( 1317): [JavaScript Error: "TypeError: cancelButton is undefined" {file: "app://communications.gaiamobile.org/contacts/gaia_build_import.js" line: 156}]

Probably because a call is done too soon before the variables are initialized.

I'll continue this on monday but I think we'll probably want this on v1.0.1.

[1] https://github.com/julienw/gaia/commit/1ff72f03ca42d8fae3003cfd44969424f8d50682

Comment 25

[Michael Vines [:m1] [:evilmachines]]

(re: tef? -- sorry, it's a little unclear to me what the issue is on v1.0.1 with all the mixed master/v1-train/v1.0.1 comments above.  Could you dumb this down for me please?)

Comment 26

[Michael Vines [:m1] [:evilmachines]]

(tef- because "we'll probably want this on v1.0.1" isn't strong enough justification.  Feel free to re-nom with more details as they become available)

Comment 27

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Facebook import is broken on any branch with enabled gaia optimization because of a race condition.

There are 2 ways of solving this :

* blacklisting the optimization for the whole communication app (it means: dialer, contacts, ftu). The problem I see with this is that the bug I found is a real bug, and disabling optimization merely hides it, but it can still occur sometimes. And I don't like disabling optimization for all this because of a bug that needs to be fixed.

* really solving the root of this bug. That's what I started to do yesterday, and what should have been done since this bug was filed in February. The truth is that I'm quite upset that I have to fix that now.

Michael, is this explanation clearer ? :-)

Comment 28

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] from comment #24)
> I have a clue on what fails: because of the optimization, the JS in
> fb_oauth.html runs faster than before, and now it runs before the second
> frame is loaded. Therefore, the various bindings that it does at load (the
> various "document.getElementById") fail and get null instead of the elements.
> 
> I have some code to change that [1] and now it fails later with :
> 
> E/GeckoConsole( 1317): [JavaScript Error: "TypeError: cancelButton is
> undefined" {file:
> "app://communications.gaiamobile.org/contacts/gaia_build_import.js" line:
> 156}]
> 
> Probably because a call is done too soon before the variables are
> initialized.
> 
> I'll continue this on monday but I think we'll probably want this on v1.0.1.
> 
> [1]
> https://github.com/julienw/gaia/commit/
> 1ff72f03ca42d8fae3003cfd44969424f8d50682

Julien,

First of all many thanks for spotting the race condition problem and for being proactive in solving it. Probably you are right and we have blamed the opt code without investigating more. 

Please let us know if you want us to take over this and will reassign it to Cristian, though he and myself are on bank holiday till Wednesday 21st. 

Nonetheless, thanks and let me know.

Comment 29

[Jose Manuel Cantera]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #26)
> (tef- because "we'll probably want this on v1.0.1" isn't strong enough
> justification.  Feel free to re-nom with more details as they become
> available)

Michael, this needs to be solved, because currently FB integration is broken in v1.0.1. We can open a separate bug but the problem, has to be solved, as per Julien's comments.

Comment 30

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Jose, if you're in holiday I will just try to solve it monday, and wait for your return to review this. Does that sound good to you ?

Comment 31

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] from comment #30)
> Jose, if you're in holiday I will just try to solve it monday, and wait for
> your return to review this. Does that sound good to you ?

perfect!, thanks

Comment 32

[Michael Vines [:m1] [:evilmachines]]

(thanks guys, tef+ per comment 28 and comment 29)

Comment 33

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I couldn't work on it since last week after all.

My current work is in : https://github.com/mozilla-b2g/gaia/pull/8721

but I'm not very happy with this. I think it would be better to expose a |ready| method from the Curtain object (kind of like mozl10n); this method would either call the callback immediately if the curtain is already ready, or wait for the DOMContentLoaded event in the frame. And then the various curtain clients would use this ready function before doing anything.

If that sounds fine to you then feel free to take over this :)

Comment 34

[Jose Manuel Cantera]

ok, thanks, Cristian will take over

Comment 35

[Jose Manuel Cantera]

the work is now tracked by bug 852598