703421 - Implement the draw metadata provider to fix races

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Patrick Walton (:pcwalton)]

This is an offshoot of bug 701623. The "draw metadata provider" is a JavaScript function that is called by widget on every draw event. In Fennec, this allows us to retrieve the viewport information (where the browser is scrolled to, currently) and send it synchronously with the draw call, so that Java doesn't have to guess where the viewport is. This fixes a lot of races.

This patch implements this, and also cleans up a few races in uploading texture data.

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Patch attached.

Comment 2

[Patrick Walton (:pcwalton)]

Comment on attachment 575313 [details] [diff] [review]
Proposed patch.

Adding dougt to the review for the widget code.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 575313 [details] [diff] [review]
Proposed patch.

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

These widget changes can land on m-c now with the fixups.

::: widget/src/android/nsWindow.cpp
@@ +1185,5 @@
>      } else {
>          DrawTo(targetSurface, ae->Rect());
> +
> +        nsAutoString emptyString;
> +        nsAString &metadata(emptyString);

Do this:

nsAutoString metadata;

@@ +1190,5 @@
> +        {
> +            nsCOMPtr<nsIAndroidDrawMetadataProvider> metadataProvider =
> +                AndroidBridge::Bridge()->GetDrawMetadataProvider();
> +            if (metadataProvider)
> +                metadataProvider->GetDrawMetadata(metadata);

and GetDrawMetadata(getter_copies(metadata))

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/829ca1d7009f

Leaving open, since the attached patch has pending review and appears to be a superset of what landed.

Comment 5

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

Comment on attachment 575313 [details] [diff] [review]
Proposed patch.

>         layerController.setOnTouchListener(new View.OnTouchListener() {
>             public boolean onTouch(View view, MotionEvent event) {
>+                if (layerController == null || event == null)
>+                    return true;
>+

layerController can never be null here, since .setOnTouchListener is called on it just above.

>+    private void adjustViewportWithThrottling() {
>+        if (!getLayerController().getRedrawHint())
>+            return;
>+
>+        if (System.currentTimeMillis() < mLastViewportChangeTime + MIN_VIEWPORT_CHANGE_DELAY) {
>+            if (mViewportRedrawTimer != null)
>+                return;
>+
>+            mViewportRedrawTimer = new Timer();
>+            mViewportRedrawTimer.schedule(new TimerTask() {
>+                @Override
>+                public void run() {
>+                    mViewportRedrawTimer = null;
>+                    adjustViewportWithThrottling();
>+                }
>+            }, MIN_VIEWPORT_CHANGE_DELAY);
>+            return;
>+        }
>+
>+        adjustViewport();
>+    }

I spent a long time staring at this before convincing myself it was correct and didn't introduce errors from possible race conditions. I don't really like the creation of a new Timer here, I'd prefer queuing the runnable such that this code is always executed from the same thread. Not sure offhand where all render() gets called from, though, so I don't know if that's possible. The current code works though, so I won't r- it, but we'll have to be careful not to break it.

>+    public void beginTransaction() {
>+        if (mTransactionLock.isHeldByCurrentThread())
>+            throw new RuntimeException("Nested transactions are not supported");
>+        mTransactionLock.lock();
>+        mInTransaction = true;
>+    }

A comment here indicating that lock() might block, and that this should never be called on the main UI thread would be good.

> 
> package org.mozilla.gecko.gfx;
> 
>+import org.mozilla.gecko.gfx.CairoImage;
> import org.mozilla.gecko.gfx.IntSize;
> import org.mozilla.gecko.gfx.LayerController;
> import org.mozilla.gecko.gfx.TileLayer;
> import javax.microedition.khronos.opengles.GL10;
> import java.nio.FloatBuffer;

You don't actually need to import classes if they're in the same package. So here all the gecko.gfx.* imports are unnecessary.

> package org.mozilla.gecko.gfx;
> 
>+import org.mozilla.gecko.gfx.CairoGLInfo;
> import org.mozilla.gecko.gfx.CairoImage;
> import org.mozilla.gecko.gfx.IntSize;
> import org.mozilla.gecko.gfx.Layer;
> import org.mozilla.gecko.gfx.TextureReaper;

Same, unnecessary imports.

>+    public TileLayer(boolean repeat, CairoImage image) {
>+        super();

Call to super() is implied, no need to do it explicitly.

>+        if (position > viewBuffer.limit()) {
>+            Log.e("Fennec", "### Position outside tile! " + dirtyRect.top);
>+            return;

please use the LOGTAG format that I switched everything over to in https://hg.mozilla.org/projects/birch/rev/a6a429a48e60

Comment 6

[Patrick Walton (:pcwalton)]

(In reply to Kartikaya Gupta (:kats) from comment #5)
> Comment on attachment 575313 [details] [diff] [review] [diff] [details] [review]
> I spent a long time staring at this before convincing myself it was correct
> and didn't introduce errors from possible race conditions. I don't really
> like the creation of a new Timer here, I'd prefer queuing the runnable such
> that this code is always executed from the same thread. Not sure offhand
> where all render() gets called from, though, so I don't know if that's
> possible. The current code works though, so I won't r- it, but we'll have to
> be careful not to break it.

I added a post to the UI thread.

Comment 7

[Patrick Walton (:pcwalton)]

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