754233 - Tab unresponsive on double-tap zoom

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Valentin Gosu [:valentin] (he/him)]

Attachment: Adb logcat logs

Version: Aurora 14.0a2 (10.05.2012 build)
Android version: 4.0.4 Cyanogen Mod 9 (04.05.2012 build)
Phone type: Samsung Galaxy S2

What I did:

Visited following webpage:
http://www.geek.com/articles/gadgets/students-create-a-watch-perception-time-20120510/
Waited for page to load.
Double-tap on text. Tab froze. Tried to get it to work properly. Failed.

What happened:

After I double-tapped the text, the webpage froze. I was unable to pan, zoom, or click anything. Reloading the page, only shows a black screen.
The tab would recover only if I killed the process and opened the page again.

What should have happened:
Should be able to pan, zoom, and click stuff after the double tap.
Should be able to reload page and have it in working order again.


Link to video:
https://picasaweb.google.com/lh/photo/ThKRMNF8Ng0f49Vk5hsmu5G5gDMRPWRAkTl9M47DkcQ?feat=directlink

Attached adb logs

Comment 1

[JP Rosevear [:jpr]]

I can reproduce this on a Galaxy Nexus.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

This loop in TiledThebesLayerOGL::RenderLayer goes into an infinite loop:

  for (size_t x = visibleRect.x; x < visibleRect.x + visibleRect.width;) {

initial values:
visibleRect.x = -14
visibleRect.width = 9

Comment 3

[Benoit Girard (:BenWa)]

+1 for release blocker. I'll look into this.

Comment 4

[Wendy East Coast USA]

I also can reproduce this on HTC Android Incredible (ADR6300) running Gingerbread 2.3.4.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Handle negative tiled layer positions

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Handle negative tiled layer positions

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Handle negative tiled layer positions (fix tile boundary bug)

Comment 9

[Ali Juma [:ajuma]]

Comment on attachment 624569 [details] [diff] [review]
Handle negative tiled layer positions (fix tile boundary bug)

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

::: gfx/layers/TiledLayerBuffer.h
@@ +107,5 @@
>      mLastPaintRegion = aLastPaintRegion;
>    }
>  
> +  // Given a position i, this function returns the position inside the current tile.
> +  int GetTileStart(int i) const { return (i >= 0) ? (i % GetTileLength()) : ((GetTileLength() - (-i % GetTileLength())) % 256); }

Use GetTileLength() instead of 256.

@@ +156,5 @@
>     */
>    virtual void PaintedTiledLayerBuffer(const BasicTiledLayerBuffer* aTiledBuffer) = 0;
>  };
>  
> +static int floor_div(int a, int b)

It'd be nice to have some documentation or a reference for the algorithm.

@@ +163,5 @@
> +  int div = a/b;
> +  if (!rem)
> +    a = b;
> +  int sub = a ^ b;
> +  sub >>= 31;

This assumes that ints are 32 bits. We should make this more portable and use 8*sizeof(int)-1 instead (or just use int32_t everywhere instead of int).

@@ +307,5 @@
>    // of no use and can be recycled.
>    // We also know that any place holder tile in the new buffer must be
>    // allocated.
>    tileX = 0;
> +  printf_stderr("Update %i, %i, %i, %i\n", newBound.x, newBound.y, newBound.width, newBound.height);

This should be #ifdef GFX_TILEDLAYER_PREF_WARNINGS.

@@ +363,5 @@
>        nsIntPoint tileOrigin(tileStartX, tileStartY);
>        newTile = AsDerived().ValidateTile(newTile, nsIntPoint(tileStartX, tileStartY),
>                                           tileDrawRegion);
>        NS_ABORT_IF_FALSE(!IsPlaceholder(newTile), "index out of range");
> +printf_stderr("Sotre Validate tile %i, %i -> %i\n", tileStartX, tileStartY, index);

#ifdef GFX_TILEDLAYER_PREF_WARNINGS

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +135,5 @@
>                                              const nsIntPoint& aTileOrigin,
>                                              const nsIntRect& aDirtyRect)
>  {
>    if (aTile == GetPlaceholderTile()) {
> +    printf_stderr("Validate tile %i, %i\n", aTileOrigin.x, aTileOrigin.y);

#ifdef GFX_TILEDLAYER_PREF_WARNINGS

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ +277,5 @@
> +    int32_t tileStartY;
> +    if (tile->mTileOrigin.x >= 0) {
> +      tileStartX = tile->mTileOrigin.x % tile->mTileSize;
> +    } else {
> +      tileStartX = tile->mTileSize - (-tile->mTileOrigin.x % tile->mTileSize);

Need to mod the result by tile->mTileSize (e.g. consider what happens when tile->mTileOrigin.x is -tile->mTileSize).

@@ +282,5 @@
> +    }
> +    if (tile->mTileOrigin.y >= 0) {
> +      tileStartY = tile->mTileOrigin.y % tile->mTileSize;
> +    } else {
> +      tileStartY = tile->mTileSize - (-tile->mTileOrigin.y % tile->mTileSize);

Mod the result by tile->mTileSize.

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +243,2 @@
>      rowCount++;
> +    printf("x=%i, visibleRect.x=%i, visibleRect.width=%i\n", x, visibleRect.x, visibleRect.width);

#ifdef GFX_TILEDLAYER_PREF_WARNINGS

Comment 10

[Benoit Girard (:BenWa)]

Attachment: Handle negative tiled layer positions (fix tile boundary bug)

Carry forward r=ajuma

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Updated patch with more floor_div calls to avoid crashing

Comment 12

[Ali Juma [:ajuma]]

Comment on attachment 624837 [details] [diff] [review]
Updated patch with more floor_div calls to avoid crashing

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

::: gfx/layers/TiledLayerBuffer.h
@@ +172,5 @@
> +    // If the signs are different substract 1.
> +    int sub;
> +    sub = a ^ b;
> +    // The results of this shift is either 0 or -1.
> +    sub >>= 31;

I still don't like using 31 here (unless we use int32_t instead of int).

Comment 13

[Benoit Girard (:BenWa)]

Attachment: Updated patch with more floor_div calls to avoid crashing

r=ajuma

Comment 14

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/670dea9cdb08

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/670dea9cdb08

Comment 16

[Alfredos-Panagiotis Damkalis [:fredy]]

I verify that this bug it still affects beta version (v14) but doesn't the nightly one (v15 18 may).

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 624847 [details] [diff] [review]
Updated patch with more floor_div calls to avoid crashing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 739679
User impact if declined: Painting stops
Testing completed (on m-c, etc.): verified on m-c
Risk to taking this patch (and alternatives if risky): low, fixes flawed calculations to handle negative coordinate.
String or UUID changes made by this patch: none

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 624847 [details] [diff] [review]
Updated patch with more floor_div calls to avoid crashing

Please land this ASAP for greater testing.

Comment 19

[Benoit Girard (:BenWa)]

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

Comment 20

[Catalin Suciu [:csuciu]]

I am not able to reproduce this issue on:
Nightly 15.0a1 (2012-05-24)
Aurora 14.0a2 (2012-05-24)
Beta 14.0b3 build2