nsObjectFrame doesn't properly handle NPP_New failing for NP_FULL [@libgobject-2.0.so.0.2000.1@0x29302 ]

RESOLVED FIXED

Status

()

Core
Plug-ins
P2
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: timeless, Assigned: jst)

Tracking

({crash, testcase})

Trunk
x86
Linux
crash, testcase
Points:
---
Bug Flags:
blocking1.9.2 -

Firefox Tracking Flags

(blocking1.9.2 .2+, status1.9.2 .2-fixed, blocking1.9.1 .9+, status1.9.1 .9-fixed)

Details

(Whiteboard: [sg:critical?][to be fixed by 533030], crash signature, URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 376052 [details] [diff] [review]
patch to create a test plugin

steps:
1. find a plugin that returns an error from NPP_New for mode=NP_FULL (you can apply the patch from this comment and build in modules/plugin/test to get one)
2. load your full page plugin, e.g.: data:application/x-test,hi
3. resize the window

Actual Results:

About to create new xtbin of 992 X 721 from (nil)...
firefox-bin: mozilla-central/widget/src/gtkxtbin/gtk2xtbin.c:316: gtk_xtbin_new: Assertion `parent_window != ((void *)0)' failed.

Program received signal SIGABRT, Aborted.
0xb7f1c7f2 in ?? () from /lib/ld-linux.so.2
(gdb) bt
#0  0xb7f1c7f2 in ?? () from /lib/ld-linux.so.2
#1  0xb71a6085 in raise () from /lib/tls/i686/cmov/libc.so.6
#2  0xb71a7a01 in abort () from /lib/tls/i686/cmov/libc.so.6
#3  0xb719f10e in __assert_fail () from /lib/tls/i686/cmov/libc.so.6
#4  0xb6da3e79 in gtk_xtbin_new (parent_window=0x0, f=0x0) at mozilla-central/widget/src/gtkxtbin/gtk2xtbin.c:316
#5  0xb4f16590 in nsPluginNativeWindowGtk2::CreateXtWindow (this=0xa73eaba0) at mozilla-central/modules/plugin/base/src/nsPluginNativeWindowGtk2.cpp:408
#6  0xb4f169d4 in nsPluginNativeWindowGtk2::CallSetWindow (this=0xa73eaba0, aPluginInstance=@0xbfa0caf8) at mozilla-central/modules/plugin/base/src/nsPluginNativeWindowGtk2.cpp:222
#7  0xb52f7e73 in nsObjectFrame::CallSetWindow (this=0xa98663a0) at mozilla-central/layout/generic/nsObjectFrame.cpp:989
#8  0xb52f835a in nsObjectFrame::DidReflow (this=0xa98663a0, aPresContext=0xa968b800, aReflowState=0xbfa0cba0, aStatus=1) at mozilla-central/layout/generic/nsObjectFrame.cpp:1094
#9  0xb52ec3ad in nsLineLayout::ReflowFrame (this=0xbfa0ceb4, aFrame=0xa98663a0, aReflowStatus=@0xbfa0cdbc, aMetrics=0x0, aPushedFrame=@0xbfa0cdb8)
    at mozilla-central/layout/generic/nsLineLayout.cpp:990
#10 0xb527db44 in nsBlockFrame::ReflowInlineFrame (this=0xa98662ec, aState=@0xbfa0d2b4, aLineLayout=@0xbfa0ceb4, aLine={mCurrent = 0xa98663e8, mListLink = 0xa986632c}, aFrame=0xa98663a0, 
    aLineReflowStatus=0xbfa0ce68) at mozilla-central/layout/generic/nsBlockFrame.cpp:3594
#11 0xb5283d72 in nsBlockFrame::DoReflowInlineFrames (this=0xa98662ec, aState=@0xbfa0d2b4, aLineLayout=@0xbfa0ceb4, aLine={mCurrent = 0xa98663e8, mListLink = 0xa986632c}, aKeepReflowGoing=0xbfa0d228, 
    aLineReflowStatus=0xbfa0cf78, aAllowPullUp=1) at mozilla-central/layout/generic/nsBlockFrame.cpp:3415
#12 0xb52845ad in nsBlockFrame::ReflowInlineFrames (this=0xa98662ec, aState=@0xbfa0d2b4, aLine={mCurrent = 0xa98663e8, mListLink = 0xa986632c}, aKeepReflowGoing=0xbfa0d228)
    at mozilla-central/layout/generic/nsBlockFrame.cpp:3262
#13 0xb5284b45 in nsBlockFrame::ReflowLine (this=0xa98662ec, aState=@0xbfa0d2b4, aLine={mCurrent = 0xa98663e8, mListLink = 0xa986632c}, aKeepReflowGoing=0xbfa0d228)
    at mozilla-central/layout/generic/nsBlockFrame.cpp:2317
#14 0xb5285393 in nsBlockFrame::ReflowDirtyLines (this=0xa98662ec, aState=@0xbfa0d2b4) at mozilla-central/layout/generic/nsBlockFrame.cpp:1897
#15 0xb52867d3 in nsBlockFrame::Reflow (this=0xa98662ec, aPresContext=0xa968b800, aMetrics=@0xbfa0d85c, aReflowState=@0xbfa0d6e8, aStatus=@0xbfa0d9ac)
    at mozilla-central/layout/generic/nsBlockFrame.cpp:944
#16 0xb528a5a9 in nsBlockReflowContext::ReflowBlock (this=0xbfa0d838, aSpace=@0xbfa0d8d0, aApplyTopMargin=1, aPrevMargin=@0xbfa0ddbc, aClearance=0, aIsAdjacentWithTop=1, aLine=0xa9866410, aFrameRS=@0xbfa0d6e8, 
    aFrameReflowStatus=@0xbfa0d9ac, aState=@0xbfa0dd34) at mozilla-central/layout/generic/nsBlockReflowContext.cpp:310
#17 0xb528255f in nsBlockFrame::ReflowBlockFrame (this=0xa98660c8, aState=@0xbfa0dd34, aLine={mCurrent = 0xa9866410, mListLink = 0xa9866108}, aKeepReflowGoing=0xbfa0dca8)
    at mozilla-central/layout/generic/nsBlockFrame.cpp:2990
#18 0xb528488d in nsBlockFrame::ReflowLine (this=0xa98660c8, aState=@0xbfa0dd34, aLine={mCurrent = 0xa9866410, mListLink = 0xa9866108}, aKeepReflowGoing=0xbfa0dca8)
    at mozilla-central/layout/generic/nsBlockFrame.cpp:2262
#19 0xb5285393 in nsBlockFrame::ReflowDirtyLines (this=0xa98660c8, aState=@0xbfa0dd34) at mozilla-central/layout/generic/nsBlockFrame.cpp:1897
#20 0xb52867d3 in nsBlockFrame::Reflow (this=0xa98660c8, aPresContext=0xa968b800, aMetrics=@0xbfa0e204, aReflowState=@0xbfa0e15c, aStatus=@0xbfa0e468)
    at mozilla-central/layout/generic/nsBlockFrame.cpp:944
#21 0xb5297c79 in nsContainerFrame::ReflowChild (this=0xa9cfaa90, aKidFrame=0xa98660c8, aPresContext=0xa968b800, aDesiredSize=@0xbfa0e204, aReflowState=@0xbfa0e15c, aX=0, aY=0, aFlags=0, aStatus=@0xbfa0e468, 
    aTracker=0x0) at mozilla-central/layout/generic/nsContainerFrame.cpp:821
#22 0xb52cedd8 in CanvasFrame::Reflow (this=0xa9cfaa90, aPresContext=0xa968b800, aDesiredSize=@0xbfa0e4c8, aReflowState=@0xbfa0e39c, aStatus=@0xbfa0e468)
    at mozilla-central/layout/generic/nsHTMLFrame.cpp:656
#23 0xb5297c79 in nsContainerFrame::ReflowChild (this=0xa9844010, aKidFrame=0xa9cfaa90, aPresContext=0xa968b800, aDesiredSize=@0xbfa0e4c8, aReflowState=@0xbfa0e39c, aX=0, aY=0, aFlags=3, aStatus=@0xbfa0e468, 
---Type <return> to continue, or q <return> to quit---q
aTrackQuit
(gdb) up
#1  0xb71a6085 in raise () from /lib/tls/i686/cmov/libc.so.6
(gdb) 
#2  0xb71a7a01 in abort () from /lib/tls/i686/cmov/libc.so.6
(gdb) 
#3  0xb719f10e in __assert_fail () from /lib/tls/i686/cmov/libc.so.6
(gdb) 
#4  0xb6da3e79 in gtk_xtbin_new (parent_window=0x0, f=0x0) at mozilla-central/widget/src/gtkxtbin/gtk2xtbin.c:316
316	  assert(parent_window != NULL);
Current language:  auto; currently c
(gdb) p parent_window
$37 = (GdkWindow *) 0x0
(gdb) up
#5  0xb4f16590 in nsPluginNativeWindowGtk2::CreateXtWindow (this=0xa73eaba0) at mozilla-central/modules/plugin/base/src/nsPluginNativeWindowGtk2.cpp:408
408	  mSocketWidget = gtk_xtbin_new(gdkWindow, 0);
Current language:  auto; currently c++
(gdb) 
#6  0xb4f169d4 in nsPluginNativeWindowGtk2::CallSetWindow (this=0xa73eaba0, aPluginInstance=@0xbfa0caf8) at mozilla-central/modules/plugin/base/src/nsPluginNativeWindowGtk2.cpp:222
222	          CreateXtWindow();
(gdb) 
#7  0xb52f7e73 in nsObjectFrame::CallSetWindow (this=0xa98663a0) at mozilla-central/layout/generic/nsObjectFrame.cpp:989
989	  window->CallSetWindow(pi);
(gdb)

For reference, the instantiate path is something like this:

#0  0xb2afb67c in nsObjectFrame::InstantiatePlugin (this=0xac3603a0, aPluginHost=0xb0ecf944, aMimeType=0xa9bfbd38 "application/x-test", aURI=0xac247180)
    at mozilla-central/layout/generic/nsObjectFrame.cpp:896
(More stack frames follow...)
(gdb) bt
#0  0xb2afb67c in nsObjectFrame::InstantiatePlugin (this=0xac3603a0, aPluginHost=0xb0ecf944, aMimeType=0xa9bfbd38 "application/x-test", aURI=0xac247180)
    at mozilla-central/layout/generic/nsObjectFrame.cpp:896
#1  0xb2b020e5 in nsObjectFrame::Instantiate (this=0xac3603a0, aMimeType=0xa9bfbd38 "application/x-test", aURI=0xac247180) at mozilla-central/layout/generic/nsObjectFrame.cpp:1808
#2  0xb2eadce3 in nsPluginStreamListener::SetupPlugin (this=0xa9bdba80) at mozilla-central/content/html/document/src/nsPluginDocument.cpp:192
#3  0xb2eadd77 in nsPluginStreamListener::OnStartRequest (this=0xa9bdba80, request=0xa9b5f220, ctxt=0x0) at mozilla-central/content/html/document/src/nsPluginDocument.cpp:114
#4  0xb2886752 in nsDocumentOpenInfo::OnStartRequest (this=0xa9bfbd90, request=0xa9b5f220, aCtxt=0x0) at mozilla-central/uriloader/base/nsURILoader.cpp:290
#5  0xb5f93c7d in nsBaseChannel::OnStartRequest (this=0xa9b5f1f0, request=0xac3a90a0, ctxt=0x0) at mozilla-central/netwerk/base/src/nsBaseChannel.cpp:665
#6  0xb5fa7517 in nsInputStreamPump::OnStateStart (this=0xac3a90a0) at mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:439
#7  0xb5fa82eb in nsInputStreamPump::OnInputStreamReady (this=0xac3a90a0, stream=0xac34d34c) at mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:395
#8  0xb7c3054a in nsInputStreamReadyEvent::Run (this=0xa9bdb5e0) at mozilla-central/xpcom/io/nsStreamUtils.cpp:111
#9  0xb7c5f533 in nsThread::ProcessNextEvent (this=0xb6b7b830, mayWait=1, result=0xbf9589d0) at mozilla-central/xpcom/threads/nsThread.cpp:510
#10 0xb7bea779 in NS_ProcessNextEvent_P (thread=0xb6b7b830, mayWait=1) at nsThreadUtils.cpp:230
#11 0xb4ed0992 in nsBaseAppShell::Run (this=0xb6a551f0) at mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:170
#12 0xb4c3bce1 in nsAppStartup::Run (this=0xb6aa0790) at mozilla-central/toolkit/components/startup/src/nsAppStartup.cpp:192
#13 0xb7eb9860 in XRE_main (argc=1, argv=0xbf958ff4, aAppData=0xb6b0e380) at mozilla-central/toolkit/xre/nsAppRunner.cpp:3232
#14 0x080499f2 in main (argc=1, argv=0xbf958ff4) at mozilla-central/browser/app/nsBrowserApp.cpp:156
(gdb)

(they're from different sessions)
(Reporter)

Comment 1

8 years ago
Created attachment 376087 [details] [diff] [review]
handle failure from NPP_New for NP_FULL

note that the web page will be empty (this is partially to fix the crash).

While it might be nice to navigate back to a previous page, i'm unwilling to do that to fix this crash. There's the potential for an infinite loop:

1. user loads page <x>
1' @page <x>
2. page <x> redirects to application/vnd.plugin.type
3. plugin returns failure code
4. plugindocument navigates back to previous page
5. browser reloads page <x>
=> go to 1'

I'd like
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #376087 - Flags: superreview?
Attachment #376087 - Flags: review?(joshmoz)

Updated

8 years ago
Attachment #376087 - Flags: superreview?

Updated

8 years ago
Attachment #376087 - Flags: review?(joshmoz) → review?(roc)

Comment 2

8 years ago
Comment on attachment 376087 [details] [diff] [review]
handle failure from NPP_New for NP_FULL

I'm fine with the "modules/plugin" part of this patch but I don't thing I'm a good reviewer for the rest, which is the bulk of it.

Passing review to roc, get sr from jst.
Comment on attachment 376087 [details] [diff] [review]
handle failure from NPP_New for NP_FULL

This is all jst
Attachment #376087 - Flags: superreview?(jst)
Attachment #376087 - Flags: review?(roc)
Attachment #376087 - Flags: review?(jst)
Keywords: testcase
Whiteboard: [sg:dos]
e.g. bp-7125c6bd-1ba5-4c50-85b0-1ad862090817
Summary: nsObjectFrame doesn't properly handle NPP_New failing for NP_FULL → nsObjectFrame doesn't properly handle NPP_New failing for NP_FULL [@libgobject-2.0.so.0.2000.1@0x29302 ]
Blocks: 510963
Bug 510963 suggests that this is a regression.
Flags: blocking1.9.2?

Comment 6

8 years ago
This is a pretty reliable URL to reproduce...
https://developer.mozilla.org/en/Build_Documentation#Get_the_source

You might have to back-button a couple of times. Scrolling up and down a few times or switching to another tab and back will render the page.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a2pre) Gecko/20090821 Namoroka/3.6a2pre

Comment 7

8 years ago
^Aaaa, crud. Comment #6 is supposed to go to Bug #509278.
Group: core-security
Whiteboard: [sg:dos] → [sg:critical?]
No longer blocks: 510963
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
(Assignee)

Updated

8 years ago
Attachment #376087 - Flags: superreview?(jst)
Attachment #376087 - Flags: superreview+
Attachment #376087 - Flags: review?(jst)
Attachment #376087 - Flags: review+
(Assignee)

Comment 9

8 years ago
I don't see how this is critical for 1.9.2. blocking-, please re-nominate and explain why this should block if you think it should.
Flags: blocking1.9.2+ → blocking1.9.2-
Comment on attachment 376087 [details] [diff] [review]
handle failure from NPP_New for NP_FULL

Rather thoroughly rotted, since it patches a file that no longer exists.
Attachment #376087 - Attachment is obsolete: true
Whiteboard: [sg:critical?] → [sg:critical?] [timeless: needs new patch]
This has been placed on our Top Security Bugs list.  Please treat it as a priority.
Note that timeless almost never looks at a bug again after he attaches a patch, so if you mean that in a way other than "has been automatically placed on" you'll probably want to treat finding someone other than him to retriage it and rewrite the patch as a priority.
jst, can you help find someone to work on this, then?
Assignee: timeless → jst
(Assignee)

Comment 14

8 years ago
This would be fixed by taking the fix for bug 533030 for 1.9.2. This already fixed on trunk.
Whiteboard: [sg:critical?] [timeless: needs new patch] → [sg:critical?]
(Assignee)

Comment 15

8 years ago
This sg:critical bug is fixable by taking the trivial fix for bug 533030, so I
think we should take this on the 1.9.2 branch.

Also, since the fix for bug 533030 already landed on trunk, I'm marking this
bug fixed.
status1.9.2: --- → ?
blocking1.9.1: --- → ?
blocking1.9.2: --- → .2+
status1.9.1: --- → wanted
status1.9.2: ? → wanted
blocking1.9.1: ? → .9+
Depends on: 533030
(Assignee)

Comment 16

7 years ago
Actually marking this FIXED, not just saying so...
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
This blocks branches; does the patch apply? If so, should it be up for approval?
(Assignee)

Comment 18

7 years ago
beltzner, the fix for this is to take the fix for bug 533030 (which is already fixed for 1.9.2), per above comments.
(Assignee)

Updated

7 years ago
status1.9.2: wanted → .2-fixed
Ah, thanks, jst. Asking in bug 533030 for a 1.9.1 fix as well.
Whiteboard: [sg:critical?] → [sg:critical?][to be fixed by 533030]
The fix for this landed on 1.9.1.9 in bug 533030 comment 9.
status1.9.1: wanted → .9-fixed
What is the best way to test this? I don't see an automated test in either this or bug 533030.
(In reply to comment #21)
> What is the best way to test this? I don't see an automated test in either this
> or bug 533030.

Any further thoughts on this from anyone? QA is blocked here.
I don't really have any good suggestions beyond comment 0.
Testing needs a plugin that fails NPP_New.
This used to be caused by bug 510963, which won't affect 1.9.1.
Group: core-security
Crash Signature: [@libgobject-2.0.so.0.2000.1@0x29302 ]
You need to log in before you can comment on or make changes to this bug.