Closed
Bug 724781
Opened 13 years ago
Closed 13 years ago
Java related crash with deleted this | [@ nsPluginInstanceOwner::Destroy() ] [@ nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner*) ]
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(firefox11 unaffected, firefox12- unaffected, firefox13+ fixed, firefox-esr10- unaffected)
VERIFIED
FIXED
mozilla13
Tracking | Status | |
---|---|---|
firefox11 | --- | unaffected |
firefox12 | - | unaffected |
firefox13 | + | fixed |
firefox-esr10 | - | unaffected |
People
(Reporter: bc, Assigned: jimm)
References
()
Details
(Keywords: crash, regression, testcase, Whiteboard: [sg:critical][advisory-tracking+])
Crash Data
Attachments
(3 files)
(deleted),
application/x-bzip2
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
jaas
:
review+
akeybl
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
See also bug 706381
1. http://oro.bullionvault.it/Prezzo-Oro.do in Nightly/Windows 7|XP?
2. Crash Browser process.
###!!! ASSERTION: Widgets that we paint must all be display roots: 'GetDisplayRo otFor(aView) == aView', file c:/work/mozilla/builds/nightly/mozilla/view/src/nsV iewManager.cpp, line 373
For application/x-java-vm found plugin npjp2.dll
For application/java-deployment-toolkit found plugin npdeployJava1.dll
For application/x-java-vm found plugin npjp2.dll
WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file c:/work/mozill a/builds/nightly/mozilla/gfx/layers/d3d9/Nv3DVUtils.cpp, line 85
WARNING: Direct3D 9-accelerated layers are not supported on this system.: file c :/work/mozilla/builds/nightly/mozilla/gfx/layers/d3d9/LayerManagerD3D9.cpp, line 87
###!!! ASSERTION: Widgets that we paint must all be display roots: 'GetDisplayRo otFor(aView) == aView', file c:/work/mozilla/builds/nightly/mozilla/view/src/nsV iewManager.cpp, line 373
#
# A fatal error has been detected by the Java Runtime Environment:
#
# EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x6409ca3a, pid=3332, tid=1784
#
# JRE version: 6.0_30-b12
# Java VM: Java HotSpot(TM) Client VM (20.5-b03 mixed mode, sharing windows-x86 )
# Problematic frame:
# C [xul.dll+0x23ca3a]
#
# An error report file with more information is saved as:
# C:\cygwin\home\bclary\hs_err_pid3332.log
#
# If you would like to submit a bug report, please visit:
# http://java.sun.com/webapps/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#
I finally got this in vs with a deleted this
+ aOwner 0x00000000 {mRefCnt={...} _mOwningThread={...} mPluginWindow=??? ...} nsPluginInstanceOwner *
+ this 0xdddddddd {mInstanceOwner=??? mInnerView=??? mWidget={...} ...} nsObjectFrame * const
> xul.dll!nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner * aOwner=0x00000000) Line 785 + 0x6 bytes C++
xul.dll!nsPluginInstanceOwner::Destroy() Line 2747 C++
xul.dll!nsObjectLoadingContent::DoStopPlugin(nsPluginInstanceOwner * aInstanceOwner=0x03b4b250, bool aDelayedStop=false) Line 2037 C++
xul.dll!nsObjectLoadingContent::StopPluginInstance() Line 2063 + 0x15 bytes C++
xul.dll!InDocCheckEvent::Run() Line 173 C++
In windbg Windows 7
(120.d60): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=feeefeee ebx=00000000 ecx=00000000 edx=00000000 esi=0000c167 edi=003aaf00
eip=6678470f esp=003aab98 ebp=003aabb8 iopl=0 nv up ei pl nz na po nc
cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00010202
xul!nsObjectFrame::SetInstanceOwner+0xf:
6678470f 894840 mov dword ptr [eax+40h],ecx ds:0023:feeeff2e=????????
Exploitability Classification: EXPLOITABLE
Recommended Bug Title: Exploitable - User Mode Write AV starting at xul!nsObjectFrame::SetInstanceOwner+0x000000000000000f (Hash=0x14796e2c.0x2e091a53)
User mode write access violations that are not near NULL are exploitable.
In Windbg Windows XP
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=fdfdfdfd ebx=00000000 ecx=00000000 edx=00000000 esi=025abfe0 edi=0012b5cc
eip=017f470f esp=0012b278 ebp=0012b298 iopl=0 nv up ei pl nz na pe nc
cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00010206
xul!nsObjectFrame::SetInstanceOwner+0xf:
017f470f 894840 mov dword ptr [eax+40h],ecx ds:0023:fdfdfe3d=????????
Exploitability Classification: EXPLOITABLE
Recommended Bug Title: Exploitable - User Mode Write AV starting at xul!nsObjectFrame::SetInstanceOwner+0x000000000000000f (Hash=0x14796e2c.0x3a5b176e)
User mode write access violations that are not near NULL are exploitable.
Had to reload a Nightly a couple of times.
bp-599113ac-8ccb-48a2-beb7-43f4d2120207
bp-cc116c69-999f-4af9-bb55-655582120207
Reporter | ||
Comment 1•13 years ago
|
||
I can reproduce with a locally saved version. Will reduce.
Comment 0 is private:
false
Reporter | ||
Comment 2•13 years ago
|
||
tests/oro.bullionvault.it/save.html1 is the original wgetted version of the source. This version is most reliable to reproduce especially if run under windbg.
save.html2 is the result of running lithium on it. This fails with an error about not being able to create a window.
Reporter | ||
Comment 3•13 years ago
|
||
Note WinXP also crashed at 0xffffffffcdcdcdcd with signature
nsQueryInterface::operator()(nsID const&, void**) nsCOMPtr<nsIDOMEventTarget>::assign_from_qi(nsQueryInterface, nsID const&)
other urls:
http://www.bullionvault.com/gold-price-chart.do
http://oro.bullionvault.it/gold-price-chart.do
Crash Signature: [@ nsPluginInstanceOwner::Destroy() ]
[@ nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner*) ] → [@ nsPluginInstanceOwner::Destroy() ]
[@ nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner*) ]
[@ nsStyleContext::GetRuleNode() ]
[@ nsQueryInterface::operator()(nsID const&, void**) nsCOMPtr<nsIDOMEventTarget>::assign_from_qi(nsQueryInterface nsI…
Reporter | ||
Comment 4•13 years ago
|
||
http://oro.bullionvault.it/Prezzo-Oro.do
bp-8ecf1c78-9f3a-493f-9e88-36a032120223
Firefox 13.0a1 Crash Report [@ nsPluginInstanceOwner::Destroy() ]
Automation hits this a lot with frames like:
Operating system: Windows NT
5.1.2600 Service Pack 3
CPU: x86
GenuineIntel family 6 model 37 stepping 1
1 CPU
Crash reason: EXCEPTION_ACCESS_VIOLATION_WRITE
Crash address: 0xffffffffddddde1d
Thread 0 (crashed)
0 xul.dll!nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner *) [nsObjectFrame.cpp : 775 + 0x6]
eip = 0x0194714f esp = 0x0012d1d8 ebp = 0x0012d1f8 ebx = 0x00000000
esi = 0x00903750 edi = 0xc31192a2 eax = 0xdddddddd ecx = 0x00000000
edx = 0x00000000 efl = 0x00010206
Found by: given as instruction pointer in context
I've reproduced locally and am reducing now.
Reporter | ||
Comment 5•13 years ago
|
||
Reporter | ||
Comment 6•13 years ago
|
||
dveditz, windbg showed this latest test case with heap fenceposts in eax and !exploitable rates this as exploitable.
Comment 7•13 years ago
|
||
Bob, do you know whether Fx10, 11, or 12 are affected here?
status-firefox13:
--- → affected
tracking-firefox13:
--- → +
Comment 8•13 years ago
|
||
yeah, the crash address and EAX 0xdddddddd all scream trouble.
Reporter | ||
Comment 9•13 years ago
|
||
beta/11 and aurora/12 aren't from what i can tell though we have wrong thread assertions on many of the original urls for older branches so i can't be absolutely sure. fwiw, i've seen 0xcd, 0xfd, 0xdd in registers so this one is a gift that keeps on giving.
Assignee | ||
Comment 11•13 years ago
|
||
DoStopPlugin can process events, and it seem we often have multiple InDocCheckEvents in the event queue aimed at stopping the same plugin instance. I'm not sure if this is by design but it seems like a fairly common occurrence. If not we should file a follow up. Regardless this fix makes sure and residual InDocCheckEvents events can't sneak in underneath the first event and do the damage they've been doing.
Fix attached and I fired off some try builds that will post back.
Attachment #601467 -
Flags: review?(joshmoz)
Attachment #601467 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 12•13 years ago
|
||
Assignee | ||
Comment 13•13 years ago
|
||
Assignee | ||
Comment 14•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 15•13 years ago
|
||
Despite comment 9 this looks like it would apply to older branches. Does it?
status-firefox-esr10:
--- → affected
status-firefox11:
--- → affected
status-firefox12:
--- → affected
tracking-firefox-esr10:
--- → ?
tracking-firefox12:
--- → +
Target Milestone: --- → mozilla13
Comment 16•13 years ago
|
||
We should get this at least in Firefox 12
Comment 17•13 years ago
|
||
Comment on attachment 601467 [details] [diff] [review]
fix v.1
[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: Potentially exploitable security issue
Testing completed (on m-c, etc.):
Risk to taking this patch (and alternatives if risky): Trivial fix, no risk
String changes made by this patch: none
Attachment #601467 -
Flags: approval-mozilla-esr10?
Attachment #601467 -
Flags: approval-mozilla-beta?
Comment 18•13 years ago
|
||
Comment on attachment 601467 [details] [diff] [review]
fix v.1
[Triage Comment]
Approved for Beta 12 given where we are in the cycle, this is sg:crit, and low risk.
Attachment #601467 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 19•13 years ago
|
||
Comment on attachment 601467 [details] [diff] [review]
fix v.1
[Triage Comment]
Please go ahead and land on ESR branch, see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details.
Attachment #601467 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Comment 20•13 years ago
|
||
Comment on attachment 601467 [details] [diff] [review]
fix v.1
apparently this doesn't land cleanly on beta (nthomas tried it for beta4) so if an update patch can be submitted for nomination we'll get it into the next beta.
Assignee | ||
Comment 21•13 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] from comment #20)
> Comment on attachment 601467 [details] [diff] [review]
> fix v.1
>
> apparently this doesn't land cleanly on beta (nthomas tried it for beta4) so
> if an update patch can be submitted for nomination we'll get it into the
> next beta.
Sorry my secure bugmail is all messed up. What branches do we want this on? Looks like beta and ESR.
Also note that this was introduced with or we became aware of it due to Josh's work in bug 90268. It's untested without that work. It's pretty safe all the same.
Blocks: 90268
Comment 23•13 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #21)
>
> Sorry my secure bugmail is all messed up. What branches do we want this on?
> Looks like beta and ESR.
that is correct
Assignee | ||
Comment 24•13 years ago
|
||
http://mxr.mozilla.org/mozilla-beta/source/layout/generic/nsObjectFrame.cpp#2434
https://hg.mozilla.org/releases/mozilla-esr10/file/dfaa5f24bd4a/layout/generic/nsObjectFrame.cpp#l2389
This work doesn't apply to beta or esr - Josh's work hasn't landed there yet. On these repos mInstanceOwner isn't set to null after the DoStopPlugin call. I'm not comfortable adding this code to those repos, to do so would require new patches that have been tested and reviewed.
Comment 25•13 years ago
|
||
The owner *is* set to null after the other DoStopPlugin calls
http://mxr.mozilla.org/mozilla-beta/source/layout/generic/nsObjectFrame.cpp#2518
https://hg.mozilla.org/releases/mozilla-esr10/file/dfaa5f24bd4a/layout/generic/nsObjectFrame.cpp#l2500
The one you pointed at was called if flag to DoStopPlugin said to do a delayed stop. The "outer" DoStopPlugin does always release the owner. In the delayed case a reference in nsStopPluginRunnable keeps the owner alive and then that's auto-released when the nsIRunnable is.
Assignee | ||
Comment 26•13 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #25)
> The owner *is* set to null after the other DoStopPlugin calls
>
> http://mxr.mozilla.org/mozilla-beta/source/layout/generic/nsObjectFrame.
> cpp#2518
> https://hg.mozilla.org/releases/mozilla-esr10/file/dfaa5f24bd4a/layout/
> generic/nsObjectFrame.cpp#l2500
>
> The one you pointed at was called if flag to DoStopPlugin said to do a
> delayed stop. The "outer" DoStopPlugin does always release the owner. In the
> delayed case a reference in nsStopPluginRunnable keeps the owner alive and
> then that's auto-released when the nsIRunnable is.
we already have this fix pre Josh's landing, this is what 'owner' does, unless I am missing something:
2486 // Transfer the reference to the instance owner onto the stack so
2487 // that if we do end up re-entering this code, or if we unwind back
2488 // here witha deleted frame (this), we can still continue to stop
2489 // the plugin. Note that due to that, the ordering of the code in
2490 // this function is extremely important.
2491
2492 nsRefPtr<nsPluginInstanceOwner> owner;
2493 owner.swap(mInstanceOwner);
..
Isn't mInstanceOwner null at this point?
2516 owner->PrepareToStop(aDelayedStop);
2517
2518 DoStopPlugin(owner, aDelayedStop);
Comment 27•13 years ago
|
||
How can we gain confidence in whether or not FF12 is affected? We'd like to get a fix in today if it is.
Assignee | ||
Comment 28•13 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #26)
> (In reply to Daniel Veditz [:dveditz] from comment #25)
> > The owner *is* set to null after the other DoStopPlugin calls
> >
> > http://mxr.mozilla.org/mozilla-beta/source/layout/generic/nsObjectFrame.
> > cpp#2518
> > https://hg.mozilla.org/releases/mozilla-esr10/file/dfaa5f24bd4a/layout/
> > generic/nsObjectFrame.cpp#l2500
> >
> > The one you pointed at was called if flag to DoStopPlugin said to do a
> > delayed stop. The "outer" DoStopPlugin does always release the owner. In the
> > delayed case a reference in nsStopPluginRunnable keeps the owner alive and
> > then that's auto-released when the nsIRunnable is.
>
> we already have this fix pre Josh's landing, this is what 'owner' does,
> unless I am missing something:
>
> 2486 // Transfer the reference to the instance owner onto the stack so
> 2487 // that if we do end up re-entering this code, or if we unwind back
> 2488 // here witha deleted frame (this), we can still continue to stop
> 2489 // the plugin. Note that due to that, the ordering of the code in
> 2490 // this function is extremely important.
> 2491
> 2492 nsRefPtr<nsPluginInstanceOwner> owner;
> 2493 owner.swap(mInstanceOwner);
> ..
>
> Isn't mInstanceOwner null at this point?
>
> 2516 owner->PrepareToStop(aDelayedStop);
> 2517
> 2518 DoStopPlugin(owner, aDelayedStop);
The fix set mInstanceOwner to null in the code that contains the plugin related code. We were already doing this prior to the landing of bug 90268. This doesn't effect version of fx prior to that landing.
Comment 29•13 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #28)
> The fix set mInstanceOwner to null in the code that contains the plugin
> related code. We were already doing this prior to the landing of bug 90268.
> This doesn't effect version of fx prior to that landing.
Thinks Jim - changing the flags to match.
Comment 30•13 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #26)
> 2492 nsRefPtr<nsPluginInstanceOwner> owner;
> 2493 owner.swap(mInstanceOwner);
> ..
>
> Isn't mInstanceOwner null at this point?
You're right, glossed right over that .swap(). Don't need this for Fx12 Beta or ESR10
Updated•13 years ago
|
tracking-firefox11:
- → ---
Comment 31•13 years ago
|
||
Bob, can you verify that this is fixed in trunk?
Reporter | ||
Comment 32•13 years ago
|
||
I retested the url in this bug as well as other bullionvault urls where I have crashed plus urls for the listed signatures and did not reproduce this crash on any branch. Note Java has been updated on all machines as well. I would say it is verified.
Updated•12 years ago
|
Group: core-security
Updated•12 years ago
|
Keywords: regression
Updated•12 years ago
|
Whiteboard: [sg:critical] → [sg:critical][advisory-tracking+]
Comment 34•12 years ago
|
||
This caused a topcrash bug so this fix was backed out and re-implemented
in a less risky way (we hope) in bug 719117. Please help verify that the
new fix (in Nightly) does indeed fix this crash too.
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•