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

VERIFIED FIXED in Firefox -esr60

Status

()

defect
P1
critical
VERIFIED FIXED
Last year
10 months ago

People

(Reporter: philipp, Assigned: handyman)

Tracking

({crash, regression})

59 Branch
mozilla63
x86_64
Windows
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 fixed, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 verified, firefox63 verified)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

Last year
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)).
Priority: -- → P3
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.
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.
Reporter

Comment 4

Last year
https://apps.facebook.com/hititrich/ is the url that's mentioned in the comments
Crash Signature: [@ IPC::ParamTraits<mozilla::plugins::NPRemoteWindow>::Write] → [@ IPC::ParamTraits<mozilla::plugins::NPRemoteWindow>::Write] [@ IPC::EnumSerializer<T>::Write]
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?
Assignee: nobody → davidp99
Flags: needinfo?(davidp99)
Priority: P3 → P1
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
Attachment #8987633 - Flags: review?(jmathies)

Comment 7

11 months ago
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.
Attachment #8987633 - Flags: review?(jmathies) → review+
Assignee

Updated

11 months ago
Keywords: checkin-needed

Comment 8

11 months ago
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59221b30f49b
Properly initialize plugin window drawing type on Windows r=jimm
Keywords: checkin-needed

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/59221b30f49b
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Flags: needinfo?(davidp99)
Assignee

Comment 11

11 months ago
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
Flags: needinfo?(davidp99)
Attachment #8987633 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(madperson)
Flags: needinfo?(lhenry)
Attachment #8987633 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 14

11 months ago
no more of these crashes were reported after b12 till now, so things are looking good so far.
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(madperson)
Flags: needinfo?(lhenry)
Looks solid on Beta still.
Status: RESOLVED → VERIFIED
Is this worth taking for 60.2esr?
Yes, if possible. It grafts cleanly.
Flags: needinfo?(davidp99)
Assignee

Comment 18

10 months ago
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
Flags: needinfo?(davidp99)
Attachment #8987633 - Flags: approval-mozilla-esr60?
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.
Attachment #8987633 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.