578330 - Plugin tests modify DOM during painting

Status: RESOLVED WORKSFORME

(Core Graveyard :: Plug-ins, defect)

Description

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

Attachment: fix

test_npn_asynccall.html and test_npn_timers.html both rely on the test plugin evaluating JS during an asynccall or timer callback. But these callbacks can run during painting, and the evaluated JS modifies the DOM. Modifying the DOM during painting is bad, it can cause crashes (and does so a lot more often with retained layers).

I'm not sure if we should be taking steps to block all plugin usage of NPN_Evaluate during painting, but whether we should or not, these tests should not be testing that and I don't want to block retained layers on addressing this plugin issue. The attached patch modifies the tests to defer the DOM modification so it doesn't happen during painting.

Comment 1

[Benjamin Smedberg]

Wait, what? It should be perfectly normal and reasonable to evaulate JS (including DOM mutations) from an asynccall/timer callback. We shouldn't ever have painting on the stack when these are called.

What's the offending stack you currently see? Did we regress the "paints win the RPC race" code?

Comment 2

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

I couldn't reproduce it locally, but consistently on try-server I'd get assertions in at least one of these tests about DOM modifications during paint, with the retained layers patches.

Maybe I did regress bug 549888, but I don't see how I'm changing anything relevant.

I guess I'll take this patch out of my queue and see if I can still reproduce on my next try-server push.

Hmm, maybe I don't understand comments #53 and #54 in bug 549888.
https://bugzilla.mozilla.org/show_bug.cgi?id=549888#c53
If we invoke plugin painting *twice* during one paint cycle, on the same plugin, can an NPN_Evaluate be executed between those invocations of plugin painting?

Comment 3

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

Hmm, in bug 564991 part 38 I definitely am seeing NPN_Invalidate being processed during a paint. I guess bug 549888 must have regressed somehow :-(.

Comment 4

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

(In reply to comment #2)
> Hmm, maybe I don't understand comments #53 and #54 in bug 549888.
> https://bugzilla.mozilla.org/show_bug.cgi?id=549888#c53
> If we invoke plugin painting *twice* during one paint cycle, on the same
> plugin, can an NPN_Evaluate be executed between those invocations of plugin
> painting?

No, shouldn't.  If a plugin message races with a paint, processing the plugin's message is deferred to another event-loop iteration (in the simplest case).

(In reply to comment #3)
> Hmm, in bug 564991 part 38 I definitely am seeing NPN_Invalidate being
> processed during a paint. I guess bug 549888 must have regressed somehow :-(.

That doesn't make sense to me ... are we spinning a nested event loop somewhere during painting?  I agree with bsmedberg that a backtrace would help a lot.  You could also try running with MOZ_IPC_MESSAGE_LOG=1 and some printfs in relevant painting code.

Comment 5

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

OK, here's the stack I'm getting:
>	xul.dll!nsPluginInstanceOwner::InvalidateRect(_NPRect * invalidRect=0x0012b7c8)  Line 2794	C++
 	xul.dll!nsNPAPIPluginInstance::InvalidateRect(_NPRect * invalidRect=0x0012b7c8)  Line 1833 + 0x1d bytes	C++
 	xul.dll!mozilla::plugins::parent::_invalidaterect(_NPP * npp=0x0547c0d0, _NPRect * invalidRect=0x0012b7c8)  Line 1201	C++
 	xul.dll!mozilla::plugins::PluginInstanceParent::RecvNPN_InvalidateRect(const _NPRect & rect={...})  Line 459 + 0x16 bytes	C++
 	xul.dll!mozilla::plugins::PPluginInstanceParent::OnMessageReceived(const IPC::Message & __msg={...})  Line 738 + 0x11 bytes	C++
 	xul.dll!mozilla::plugins::PPluginModuleParent::OnMessageReceived(const IPC::Message & __msg={...})  Line 330 + 0x11 bytes	C++
 	xul.dll!mozilla::ipc::AsyncChannel::OnDispatchMessage(const IPC::Message & msg={...})  Line 262 + 0x1e bytes	C++
 	xul.dll!mozilla::ipc::RPCChannel::Call(IPC::Message * msg=0x04b32508, IPC::Message * reply=0x0012babc)  Line 246	C++
 	xul.dll!mozilla::plugins::PPluginInstanceParent::CallPaint(const mozilla::plugins::NPRemoteEvent & event={...}, short * handled=0x0012bb84)  Line 462 + 0x16 bytes	C++
 	xul.dll!mozilla::plugins::PluginInstanceParent::NPP_HandleEvent(void * event=0x0012bc40)  Line 647	C++
 	xul.dll!mozilla::plugins::PluginModuleParent::NPP_HandleEvent(_NPP * instance=0x0547c0d0, void * event=0x0012bc40)  Line 490	C++
 	xul.dll!nsNPAPIPluginInstance::HandleEvent(void * event=0x0012bc40, short * result=0x00000000)  Line 1378 + 0x4e bytes	C++
 	xul.dll!nsPluginInstanceOwner::Paint(const tagRECT & aDirty={...}, HDC__ * aDC=0x45010c7e)  Line 5058	C++
 	xul.dll!nsObjectFrame::PaintPlugin(nsIRenderingContext & aRenderingContext={...}, const nsRect & aDirtyRect={...}, const nsRect & aPluginRect={...})  Line 1808	C++

Comment 6

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

That was from the test plugin, using a test where I have specifically made it call NPN_Invalidate synchronously during paint.

Here's a stack from a Flash testcase:
>	xul.dll!nsPluginInstanceOwner::InvalidateRect(_NPRect * invalidRect=0x0012b064)  Line 2794	C++
 	xul.dll!nsNPAPIPluginInstance::InvalidateRect(_NPRect * invalidRect=0x0012b064)  Line 1833 + 0x1d bytes	C++
 	xul.dll!mozilla::plugins::parent::_invalidaterect(_NPP * npp=0x04f8a4a8, _NPRect * invalidRect=0x0012b064)  Line 1201	C++
 	xul.dll!mozilla::plugins::PluginInstanceParent::RecvNPN_InvalidateRect(const _NPRect & rect={...})  Line 459 + 0x16 bytes	C++
 	xul.dll!mozilla::plugins::PPluginInstanceParent::OnMessageReceived(const IPC::Message & __msg={...})  Line 738 + 0x11 bytes	C++
 	xul.dll!mozilla::plugins::PPluginModuleParent::OnMessageReceived(const IPC::Message & __msg={...})  Line 330 + 0x11 bytes	C++
 	xul.dll!mozilla::ipc::AsyncChannel::OnDispatchMessage(const IPC::Message & msg={...})  Line 262 + 0x1e bytes	C++
 	xul.dll!mozilla::ipc::RPCChannel::Call(IPC::Message * msg=0x025d7118, IPC::Message * reply=0x0012b358)  Line 246	C++
 	xul.dll!mozilla::plugins::PPluginInstanceParent::CallPaint(const mozilla::plugins::NPRemoteEvent & event={...}, short * handled=0x0012b420)  Line 462 + 0x16 bytes	C++
 	xul.dll!mozilla::plugins::PluginInstanceParent::NPP_HandleEvent(void * event=0x0012b4dc)  Line 647	C++
 	xul.dll!mozilla::plugins::PluginModuleParent::NPP_HandleEvent(_NPP * instance=0x04f8a4a8, void * event=0x0012b4dc)  Line 490	C++
 	xul.dll!nsNPAPIPluginInstance::HandleEvent(void * event=0x0012b4dc, short * result=0x00000000)  Line 1378 + 0x4e bytes	C++
 	xul.dll!nsPluginInstanceOwner::Paint(const tagRECT & aDirty={...}, HDC__ * aDC=0x810109c1)  Line 5058	C++
 	xul.dll!nsObjectFrame::PaintPlugin(nsIRenderingContext & aRenderingContext={...}, const nsRect & aDirtyRect={...}, const nsRect & aPluginRect={...})  Line 1808	C++

Comment 7

[Benjamin Smedberg]

Well, yeah, if the plugin calls NPN_Invalidate during paint, then the plugin is probably doing something incorrect and will crash.

But that's not this bug, is it?

Comment 8

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

cjones points out that NPN_Invalidate is async and therefore not subject to race resolution, so we should be prepared to deal with them anytime. That's fine, I've already done that in new patches in bug 564991.

Next step is to do another tryserver push without the workaround patch in this bug and get some stacks for the assertion failures. Unfortunately as I recall the stacks I got were completely broken last time.

Comment 9

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

(In reply to comment #7)
> Well, yeah, if the plugin calls NPN_Invalidate during paint, then the plugin is
> probably doing something incorrect and will crash.

cjones says we can receive an asynchronous NPN_Invalidate during paint (i.e., the plugin might not have called NPN_Invalidate during paint), so we need to handle it.

Comment 10

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

Hmm, I'm not seeing those test failures in the latest tryserver run. Maybe I fixed it accidentally.