552809 - Intermittent black boxes from asyncDrawXULElement

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benjamin Stover (:stechz)]

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMCanvasRenderingContext2D.asyncDrawXULElement]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/TileManager.js :: render :: line 682"  data: no]

This only happens occasionally and I've not linked it to anything specifically going wrong.

Comment 1

[Frederic Plourde (:fred23)]

ok, seems we're getting this error systematically on win32 builds. I'm going to investigate today

Comment 2

[Frederic Plourde (:fred23)]

Attachment: Build log showing abovementionned error

Here's my build log for this fennectrolysis build under win32
(I'm not using any of Dougt's patches atm)

Comment 3

[Frederic Plourde (:fred23)]

Attachment: Screen capture

Screen capture showing current fennectrolysis bogus behavior (google.com) that I suspect is related to the present bug

Comment 4

[Oleg Romashin (:romaxa)]

I've found that this is happening due to double call of asyncDrawXulElement to the same canvas element without waiting for async response about drawFinished..

And in this case asyncDrawRequest failed in 
http://mxr.mozilla.org/projects-central/source/electrolysis/content/canvas/src/nsCanvasRenderingContext2D.cpp#3612

Comment 5

[Oleg Romashin (:romaxa)]

easily reproducible on about:license page.

Comment 6

[Doug Turner (:dougt)]

joe, any idea what is going on?

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Fennec workaround

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 453589 [details] [diff] [review]
Fennec workaround

  wrapper.ctx = aCanvas.MozGetIPCContext("2d");
>   wrapper.drawContent = function(aLeft, aTop, aWidth, aHeight, aColor, aFlags) {
>-    this.ctx.asyncDrawXULElement(aBrowser, aLeft, aTop, aWidth, aHeight, aColor, aFlags);
>+    try {
>+      this.ctx.asyncDrawXULElement(aBrowser, aLeft, aTop, aWidth, aHeight, aColor, aFlags);
>+    }
>+    catch (e) {
>+      Components.utils.reportError(e);
>+      let self = this;
>+      setTimeout(function() {
>+        self.drawContent(aLeft, aTop, aWidth, aHeight, aColor, aFlags);
>+      }, 0);
>+    }

Actually, I'd like to just call the raw asyncDrawXULElement in the catch. We could possibly start a death loop if the function keeps throwing. We should only give it one extra chance.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

I still see artifacting with this patch.

Comment 10

[Benjamin Stover (:stechz)]

Attachment: Fennec workaround v2

Mark, I doubt you will like this.  This:
1) handles async render events to ensure that the render was successful
2) if there is an error and the event never comes, it will do a retry every 500 msecs up to 4 times.

See if this helps with artifacts.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 453842 [details] [diff] [review]
Fennec workaround v2

Overall, I can live with changes like this. I do find we still have rendering problems though. On IRC, you said it was when we start on a local page (about:home) and open a remote page (news.google.com)

Comment 12

[Benjamin Stover (:stechz)]

Attachment: Fennec workaround v3

I based this patch off the assumption that canvas tiles should never render async content followed by sync content.

It could probably use cleanup, but this is dead code walking.

Let me know if you can reproduce render glitches.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 453932 [details] [diff] [review]
Fennec workaround v3

This patch definitely makes rendering better. I think we have an underlying platform problem though. Needed to destroy and recreate canvases when swithcing from drawWindow to asyncDrawXULElement seems wrong and busted.

Lastly, would it simplify anything if we just _always_ destroyed canvases when loading pages?

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

... and yes, I really don't like the hoops we need to jumpt through to make this work. TileManager has reached a new "high" in complexity.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-b:
http://hg.mozilla.org/mobile-browser/rev/1d0d9c3c481f

But not all rendering issues are fixed.

Comment 16

[Benjamin Stover (:stechz)]

Mark and I have been noticing black boxes and weird artifacts occasionally.  I believe this is a problem with the async call.

The best way I've managed to reproduce the bug is by opening the start page and google.com in separate tabs and clicking between them.

It does not look like this is due to any unreported errors in the child.  PresShell is rendering to the document correctly when this problem occurs.  Maybe there is some sort of race condition.

Comment 17

[Stuart Parmenter]

joe, still need your help here -- wan to get rid of the workaround asap.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 580197 shows some stacks where this bug (and the bandaid fix) is actually causing huge memory spikes and in some cases crashes after "too much recursion" problems.

Comment 19

[Josh Matthews [:jdm]]

There's a really easy way to trigger a failure - kill the content process in some manner, then switch to another tab.  AsyncDrawXULElement will try to get the child process, fail, check for an existing docShell and bail with NS_ERROR_FAILURE.  This almost certainly isn't the intermittent problem seen here, but it does make the browser explode due to the patch checked in here.

Comment 20

[Aakash Desai [:aakashd]]

I'm seeing this on Android and Doug says he's seen it on maemo as well.

Comment 21

[Joe Drew (not getting mail)]

(there are no NS_ERROR_FAILURE problems anymore, because the Fennec frontend code doesn't try to call asyncDrawXULElement on canvas elements that are pending another draw.)

Comment 22

[Joe Drew (not getting mail)]

Attachment: XSync after drawing in the child process

Sometimes we get the wrong results in the parent process because the child process's X drawing hadn't yet been synced to the server. This patch fixes that.

Comment 23

[Benjamin Stover (:stechz)]

Joe:

I'm finding that I still need to recreate canvases everytime I switch from the start page to another web site.  If you comment out the "recreateCanvas" in TileManager.js.in here:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/TileManager.js.in#835

You will see what I mean.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 463223 [details] [diff] [review]
XSync after drawing in the child process

r=me with comment per IRC.

Comment 25

[Joe Drew (not getting mail)]

Attachment: XSync after drawing in the child process, v2

Carrying over r+ - this version just contains the comment changes requested.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/bcd22ffaf253