1445444 - Crash in IPC::ParamTraits<mozilla::plugins::NPRemoteWindow>::Write

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-713e37db-ebaa-4228-96c0-3e0180180313.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll IPC::ParamTraits<mozilla::plugins::NPRemoteWindow>::Write dom/plugins/ipc/PluginMessageUtils.h:396
1 xul.dll mozilla::plugins::PPluginInstanceParent::CallNPP_SetWindow ipc/ipdl/PPluginInstanceParent.cpp:237
2 xul.dll mozilla::plugins::PluginInstanceParent::NPP_SetWindow dom/plugins/ipc/PluginInstanceParent.cpp:1373
3 xul.dll nsNPAPIPluginInstance::SetWindow dom/plugins/base/nsNPAPIPluginInstance.cpp:365
4 xul.dll nsPluginNativeWindow::CallSetWindow dom/plugins/base/nsPluginNativeWindow.h:64
5 xul.dll nsPluginNativeWindowWin::CallSetWindow dom/plugins/base/nsPluginNativeWindowWin.cpp:586
6 xul.dll nsPluginFrame::CallSetWindow layout/generic/nsPluginFrame.cpp:675
7 xul.dll nsPluginFrame::ReflowFinished layout/generic/nsPluginFrame.cpp:540
8 xul.dll mozilla::PresShell::HandlePostedReflowCallbacks layout/base/PresShell.cpp:4049
9 xul.dll mozilla::PresShell::DidDoReflow layout/base/PresShell.cpp:8804

=============================================================

crash reports with this signature are starting to turn up on firefox 59 from users on win64 builds and all with MOZ_RELEASE_ASSERT(EnumValidator::IsLegalValue(aValue)).

Comment 1

[Jim Mathies [:jimm]]

May only affect 59.

Comment 2

[Marcia Knous [:marcia]]

One comment (from Google translate): I would like to know what is happening with the game lucky slots that can not play does not appear.

Comment 3

[Marcia Knous [:marcia]]

This showed up in our crash spike report for 4/19 - there were 1220 crashes on that date for some reason. This does affect 60 as well, even though it is not large volume.

Many of the comments mention "Hit it rich casino" game - so perhaps there was some kind of change on Facebook or Zynga's end that caused the spike.

Comment 4

[[:philipp]]

https://apps.facebook.com/hititrich/ is the url that's mentioned in the comments

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This appears to be hitting with pretty high frequency still on release. Is this something you'd have cycles to look at by chance, David?

Comment 6

[David Parks [:handyman]]

Attachment: Properly initialize plugin window drawing type on Windows

I've got time for a quick look but if this idea doesn't work then this will probably end up back-burnered.

First off, the crash call stack makes no sense.  As usual, this is because of the optimizer.
At first, the call to nsPluginNativeWindowWin::CallSetWindow _appears_ to be acting on the assumption that its parameter, aPluginInstance, is null [1].  But it then calls the base class implementation, nsPluginNativeWindow::CallSetWindow, and passes aPluginInstance, which it then uses as it is non-null [2].  The deal here is that the call stack is lying about the line in nsPluginNativeWindowWin::CallSetWindow.  Looking at the assembler, we can see that there is a later branch of that function [3] that the optimizer has converted into a jump backward, to this code.  So... thats why the crash appears as nonsense.

The actual enum serialization complaint that causes the crash is that the window type is 0, which is not valid (valid enum values are NPWindowTypeWindow(1) and NPWindowTypeDrawable(2)).  Barring something more dramatic, this appears to be due to a lack of initialization.  I haven't yet figured out how the window type is usually set to NPWindowTypeWindow on Windows but the code path in the crash suggests that we are not async drawing (see [5] from the stack).  In non-Windows implementations, in this case, the type would have been set in the nsPluginNativeWindowImpl constructor [6] but isn't in the nsPluginNativeWindowWin version [7].  So I've added it there but I stress that I don't know for sure if this is the right spot.  Thoughts, Jimm?

Test build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41731a18288c9d39dfd8b5dbbd499813b1152feb

----

[1] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/plugins/base/nsPluginNativeWindowWin.cpp#586
[2] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/plugins/base/nsPluginNativeWindow.h#64
[3] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/plugins/base/nsPluginNativeWindowWin.cpp#602
[4] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/plugins/base/nsPluginNativeWindow.cpp#35
[5] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/layout/generic/nsPluginFrame.cpp#674
[6] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/plugins/base/nsPluginNativeWindow.cpp#35
[7] https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/plugins/base/nsPluginNativeWindowWin.cpp#482

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8987633 [details] [diff] [review]
Properly initialize plugin window drawing type on Windows

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

sorry this took so long.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59221b30f49b
Properly initialize plugin window drawing type on Windows r=jimm

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/59221b30f49b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

I don't think we're going to see any verification of this until it's on Beta. Please request approval when you get a chance.

Comment 11

[David Parks [:handyman]]

Comment on attachment 8987633 [details] [diff] [review]
Properly initialize plugin window drawing type on Windows

I was nervous about uplifting this (see comment 6) but I think if it was going to harm anything it would be obvious -- and I don't see anything new in crash-stats.  Since, as ryanvm alluded to, the crash isn't really seen in nightly, it makes sense to uplift.

Approval Request Comment
[Feature/Bug causing the regression]:
N/A.  This field was never initialized in the constructor.

[User impact if declined]:
Crash!

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
There is no known STR so its fair to say it has not.

[Needs manual test from QE? If yes, steps to reproduce]: 
Would be great but there is no known way to do this (no STR).

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Medium

[Why is the change risky/not risky?]:
Unexpected failure could lead to crashes or weird behavior around plugins, but I stress that the possibility that this patch would fail in some way _when it would not have crashed before_ is small.  In that sense, it is not risky.

[String changes made/needed]:
None

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8987633 [details] [diff] [review]
Properly initialize plugin window drawing type on Windows

The crash volume on beta is very low but on 61.0.1 is reasonable (~1200), taking it with the hopes that this improves the situation when 62 goes live, Beta62+

Liz, Marcia, Philipp, FYI, in case things look worse we should consider backing it out of m-b.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5627d6ef2916

Comment 14

[[:philipp]]

no more of these crashes were reported after b12 till now, so things are looking good so far.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Looks solid on Beta still.

Comment 16

[Julien Cristau [:jcristau]]

Is this worth taking for 60.2esr?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Yes, if possible. It grafts cleanly.

Comment 18

[David Parks [:handyman]]

Comment on attachment 8987633 [details] [diff] [review]
Properly initialize plugin window drawing type on Windows

I'm satisfied.  Lets try for esr60.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This crash was low volume but seems to have been eliminated by this simple initialization fix.

User impact if declined: 
Somewhat rare crashes (content proc) when starting Flash.  

Fix Landed on Version:
63

Risk to taking this patch (and alternatives if risky): 
Pretty Low.  The side-effects of initializing this field in the plugin object could have adverse affects in other parts of plugin initialization but we haven't seen any and, if there was anything to this, I believe it would have popped up in beta.

String or UUID changes made by this patch: 
N/A

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8987633 [details] [diff] [review]
Properly initialize plugin window drawing type on Windows

No known issues on Nightly or Beta since this landed. Approved for ESR 60.2 also.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/1973f8b39e0c