868271 - Tuenti will eventually prevent scrolling after looking at an account's upcoming events

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Devices: Unagi, Keon
Build: 3/15/2013 unagi, 4/16/2013 for keon

STR:

1. Login with an existing account that has notifications
2. Select the green notifications bar
3. Select the ... button in the top right
4. Select the calendar icon
5. Try scrolling

Expected:

The app should be able to scroll.

Actual:

The app isn't possible to scroll. Error console shows:

E/GeckoConsole(  467): [JavaScript Error: "can't access dead object" {file: "chrome://global/content/BrowserElementPanning.js" line: 459}]

Additional Notes:

We have an account that will allow you to test this directly, so feel free to privately if you need the account that reproduced this bug.

Comment 2

[Jason Smith [:jsmith]]

This impacts a preinstalled app planned for 1.01 ship.

tef nom to see if this is bad enough to block.

Comment 3

[Andrew Overholt [:overholt]]

jwatt, do you have any ideas here?

Comment 4

[(not reading bugmail) Nick Desaulniers [:\n]]

From the previous bug, we were seeing in logcat:

E/GeckoConsole(  467): [JavaScript Error: "can't access dead object" {file: "chrome://global/content/BrowserElementPanning.js" line: 459}]

Comment 5

[(not reading bugmail) Nick Desaulniers [:\n]]

I am able to reproduce this on a 1.0.1 peak, but not a 1.1.0 unagi.

Comment 6

[Jonathan Watt [:jwatt]]

Well, the info on the 'JavaScript Error: "can't access dead object"' message here:

https://blog.mozilla.org/addons/2012/09/12/what-does-cant-access-dead-object-mean/

seems to imply that code in BrowserElementPanning.js is holding a reference to a content DOM node that the content itself has no references to, so it was GC'ed leaving the chrome reference dangling. Exactly which DOM node is being held I don't know. Do the debugging tools work with 1.0.1?

Other than that, I don't have any insights. I guess I'm being asked because I've touched that file, but that was only to add an evt.multipleActionsPrevented check to a line that was checking evt.defaultPrevented. I don't actually really know anything about BrowserElementPanning.js I'm afraid.

Comment 7

[Andrew Overholt [:overholt]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> I guess I'm being asked because I've touched that file

Indeed, and Milan thought maybe you'd know more than your commit indicated :)  Thanks for looking.  Let's see if Shih-Chiang has any thoughts.

Comment 8

[(not reading bugmail) Nick Desaulniers [:\n]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> seems to imply that code in BrowserElementPanning.js is holding a reference
> to a content DOM node that the content itself has no references to, so it
> was GC'ed leaving the chrome reference dangling.

Is it possible that this could be a developer issue in their JavaScript, or a platform issue on our end?  I'd like to be able to inform the developer who is having trouble reproducing this issue, which the review team has flagged them for.

Comment 9

[Jonathan Watt [:jwatt]]

It's a problem in chrome code that is referencing content code. BrowserElementPanning.js is chrome code, and I'm assuming it's being used to pan some content page here, so the issue would be in BrowserElementPanning.js I believe. It might be worth having a chat with the guys that introduced this error message just to double check.

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

BrowserElementPanning is holding two reference to DOM object, one for the scrolling target and another for the touch event target. I'm going to find out if these DOM object been remove during the panning gesture, or we doesn't clean up the reference correctly.

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Touchend event is preventDefault'ed while tapping on the green notifications bar. Therefore, primaryPointerId is not correctly clean up after touchend. It'll block BrowserElementPanning detecting the next panning sequence.
The dead object error reveals that pointerDownTarget is also not clean up after touchend.

However, I cannot reproduce this scrolling issue using browser app since APZC will take over the panning gesture. I'll need the packaged app for verification.

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: b2g18 - should not skip touchend handler

Touchend handler should be invoked under any circumstance because several attributes need to be clean up after panning gesture is finished.

Comment 13

[Donovan Preston [:fzzzy]]

To install as an app, visit a page which contains the following javascript in the browser. You will be prompted to install the app.

  window.addEventListener('DOMContentLoaded', function () {
    navigator.mozApps.install('http://m.tuenti.com/manifest.webapp');
  });

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: m-c - should not skip touchend handler

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

Comment 15

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Tuenti app scrolling issue can be fixed by this patch.

Comment 16

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

Comment on attachment 746343 [details] [diff] [review]
m-c - should not skip touchend handler

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

::: dom/browser-element/BrowserElementPanning.js
@@ +67,2 @@
>        return;
>  

If some code needs to be cleaned up when the finger is released can you create a specific code path for it? I'm kind of worried that entering touchend even if it has been cancelled will have side effects.

Comment 17

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

Comment on attachment 745827 [details] [diff] [review]
b2g18 - should not skip touchend handler

This one is the same than the one for m-c. Once the one for m-c will be reviewed this one can inherit the review stamp.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: m-c - explictly clean up panning state if touchend has been preventDefault

Explictly clean up panning state if touchend/mouseup has been preventDefault.

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

Comment 19

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Patch for landing on b2g18

Rebase to b2g18.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/ac6df93ede35

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f16a5cda04e5
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d40d1831ff41

Comment 23

[Sarah Parsons]

Verified fixed on Inari Build ID: 20130516070202
Environmental  Variables:

Kernel Date: Feb 21
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/430b0d1ce2cc
Gaia: e98024772590f97d9443acf1af3958b111ed9cb6

Also verified fixed on the Unagi Build ID: 20130515230207

Environmental  Variables:
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/4b5fe47ad76c
Gaia: fbb9a3febbc81dc4c290b6a3bc095495fa19484c

No error occurs when scrolling through calendar events in the Tuenti app.