622985 - Plugin crash [@ gfxContext::gfxContext ] when loading "My T-Mobile" login page

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Load https://my.t-mobile.com/Login/MyTMobileLogin.aspx
 (If you don't have a plugin crash page, reload)

ACTUAL RESULTS: I get a plugin crash notification at that page about 20-50% of the time.

I also get a bunch of messages like:
> ###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv
and
> ###!!! [Parent][RPCChannel] Error: Channel error: cannot send/recv
spammed to my terminal.

Sample crash report: bp-69b5a32d-b512-484c-a06e-506eb2110104
From quick code inspection from the source links given in the crash report, I'm guessing it's because we end up using "mHelperSurface" as our surface, when it's actually null, and then we crash when we try to call a method on it.

In the gfxContext constructor, we crash at this line:
> 64     mCairo = cairo_create(surface->CairoSurface());
so it looks like |surface| is null.  That's probably because, up one level, we suddenly switch to using mHelperSurface without null-checking it:
> 2594 PluginInstanceChild::PaintRectToSurface(const nsIntRect& aRect,
[...]
> 2600     nsRefPtr<gfxASurface> renderSurface = aSurface;
[...]
> 2601 #ifdef MOZ_X11
[...]
> 2616         renderSurface = mHelperSurface;
> 2618 #endif
> 2619 
> 2620     if (mIsTransparent) {
> 2621        // Clear surface content for transparent rendering
> 2622        nsRefPtr<gfxContext> ctx = new gfxContext(renderSurface);

System information:
* Ubuntu 10.10 x86_64
* Mozilla/5.0 (X11; Linux x86_64; rv:2.0b9pre) Gecko/20110104 Firefox/4.0b9pre
* Flash version: libflashplayer-square-preview3.so // 10.3 d162

Comment 1

[Daniel Holbert [:dholbert]]

I'm assuming that it's |mHelperSurface| that's null rather than |aSurface|, because we call some methods on |aSurface| up one level in "PluginInstanceChild::PaintRectWithAlphaExtraction":

> 2671     whiteImage->SetDeviceOffset(deviceOffset);
> 2672     PaintRectToSurface(aRect, whiteImage, gfxRGBA(1.0, 1.0, 1.0));
(Note that whiteImage becomes |aSurface| in the PaintRectToSurface call)

So if |aSurface| were null, then we would have crashed back at line 2671.  But we crash later than that -- so |aSurface| is presumably non-null.

Comment 2

[Daniel Holbert [:dholbert]]

Tentatively flagging as blocking the bug that added the
 "renderSurface = mHelperSurface;"
line from comment 0.

Comment 3

[Daniel Holbert [:dholbert]]

Adding 'regression' keyword, since two old builds that I tested don't exhibit this bug:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100914 Firefox/4.0b7pre
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre

The latter build there is from after the landing of bug 556487 (which I flagged as causing this bug in comment 2), so I'm reverting comment 2.

Comment 4

[Daniel Holbert [:dholbert]]

Was able to reproduce this (or something like it) in nightlies as far back as 2010-10-03...
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20101003 Firefox/4.0b7pre
...though it's a lot less frequent there for some reason.  I got just one plugin-crash on reloading the "My T-mobile" page in that nightly...
 bp-ce80cd89-0760-447b-aa49-ad6512110104
...and I couldn't get more beyond that. (Note that the signature & stack there don't have any symbols for things in libxul -- maybe because we don't have symbols on crash-stats for builds that far back?)

Comment 5

[Oleg Romashin (:romaxa)]

Should I  be logged in on this page? or login page is enough to reproduce this bug?

MaybeCreatePlatformHelperSurface should create mHelperSurface, and mHelperSurface can be null only if we got somehow DoAsyncSetWindow between ShowPluginFrame and PaintRectToSurface...

And that actually might happen, because we get crash on rendering white image... and between ShowPluginFrame and PaintRectToSurface rendering to blackImage did happen. (which might rotate event loop and process DoAsyncSetWindow async call)

Haven't seen this before, but it might be related to bug 615417...

Comment 6

[Oleg Romashin (:romaxa)]

Possible options are:
1) Hold mHelperSurface in PaintRectWithAlphaExtraction
2) call MaybeCreatePlatformHelperSurface before each PaintRectToSurface in PaintRectWithAlphaExtraction
3) just stop rendering PaintRectWithAlphaExtraction if mHelperSurface become null

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Should I  be logged in on this page? or login page is enough to reproduce this
> bug?

No -- no need to be logged in.  Just loading the login page itself is enough. (My testing & most of my crashes so far have been me testing with fresh profile, not logged in, just loading/reloading the URL.)

Comment 8

[Daniel Holbert [:dholbert]]

FWIW, I just reproduced this on a different machine running Ubuntu 10.10, but with the stock flashplugin-installer package (version 10.1.102.65ubuntu0.10.10.1 according to synaptic / ver 10.1 r102 according to about:plugins), and with a 32-bit cpu instead of 64-bit.

So this isn't 64-bit specific, nor is it specific to the "square" preview of Flash 10.3.

crashreport for this new machine is: bp-a3538fc4-3a0f-4580-91e5-f0d722110104

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> Haven't seen this before, but it might be related to bug 615417...

Just to be clear -- if by "related to" you were suggesting "regression from": this is unlikely to be a regression from that bug, because (per comment 4) I can reproduce a plugin crash at this bug's URL in nightlies at least as far back as Oct 5th, which is 2 months before bug 615417 landed.

Comment 10

[Oleg Romashin (:romaxa)]

Hmm.. for now was not able to reproduce it on Ubuntu 10.10, Flash 10.1 r102, Built from http://hg.mozilla.org/mozilla-central/rev/be8006fc9c4a

Comment 11

[Oleg Romashin (:romaxa)]

Is it possible to analyze coredump with gdb from that crash report somehow?

Comment 12

[Oleg Romashin (:romaxa)]

Ok, got it reproducible with my build, will check what is going on.

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Possible fix version 1

Following leading to crash:
DoAsyncSetWindow - helpSurf:(nil), sz[0,0]->[941,31] Drop mHelperSurface!!
MaybeCreatePlatformHelperSurface() new helpSurf:0xaf4ee760

PaintRectWithAlphaExtraction - begin
PaintRectToSurface (blackImage)render = helpSurf:0xaf4ee790
DoAsyncSetWindow helpSurf:0xaf4ee760, sz[941,31]->[941,37] Drop mHelperSurface!!
PaintRectToSurface render:0xaf4ee6a0 = helpSurf:(nil)

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Possible fix version 2

Another version of fix for this bug

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should not be processing a DoAsyncSetWindow during PaintRectWithAlphaExtraction. What is the call stack for that DoAsyncSetWindow?

Comment 16

[Benjamin Smedberg]

Bug 624809 has some data for a similar issue on Windows.

Comment 17

[Oleg Romashin (:romaxa)]

> We should not be processing a DoAsyncSetWindow during
plugin can rotate event-loop anytime even while rendering, we even have some protection for it: mPendingPluginCall.. in PluginInstanceChild.h

Why both patches rejected here?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See bug 624809. Benjamin found the problem.

The plugin should not be spinning the event loop while processing a paint event!

Comment 19

[Oleg Romashin (:romaxa)]

should we just check for mPendingPluginCall and postpone DoAsyncSetWindow for one more iteration?

Comment 20

[Oleg Romashin (:romaxa)]

IIRC I tried to set mPendingPluginCall true/false for SetWindow call... but some tests were failing ...

Comment 21

[Benjamin Smedberg]

Was this fixed by bug 624809?

Comment 22

[Daniel Holbert [:dholbert]]

Seems to have been, yes!

Loaded ~10 "My T-Mobile" tabs in the post-bug624809-fix nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b10pre) Gecko/20110115 Firefox/4.0b10pre
and I cycled through them 4 times with no crash notifications.

I tried the previous (pre-bug624809-fix) nightly, and after cycling through & reloading all the tabs twice, most or all tabs reported plugin crashes.

Resolving as fixed by bug 624809.

Comment 23

[Daniel Holbert [:dholbert]]

There's now a visual glitch on the first load of the "My T-Mobile" page when it's in a background tab, though, and I think it might be related.  I filed Bug 622985 on that.

Comment 24

[Daniel Holbert [:dholbert]]

copy-paste fail... I meant to say "I filed bug 626691 on that."