145377 - browser crashes after clicking BACK on this url

Status: VERIFIED FIXED

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

Description

[shrirang khanzode]

seen on mozilla 1.0.0 winNT
I see some kind of odd load/reload on 4.79 ...but no crash there

steps:
load url in mozilla 1.0.0 (0517)
it loads up blank..click BACK
CRASH

stack:

nsDocShell::GetRootScrollableView 
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, line 6054] 
nsDocShell::GetCurScrollPos 
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, line 3028] 
nsDocShell::ScrollIfAnchor 
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, line 5211] 
nsDocShell::InternalLoad 
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, line 4576] 
nsDocShell::LoadURI [d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, 
line 665] 
nsDocShell::LoadURI [d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, 
line 2391] 
XPTC_InvokeByIndex 
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, 
line 106] 
XPCWrappedNative::CallMethod 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 
2028] 
XPC_WN_CallMethod 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp, 
line 1267] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 790] 
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2744] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 806] 
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2744] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 806] 
fun_apply [d:\builds\seamonkey\mozilla\js\src\jsfun.c, line 1555] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 790] 
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2744] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 806] 
js_InternalInvoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 881] 
JS_CallFunctionValue [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3426] 
nsJSContext::CallEventHandler 
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 1019] 
nsJSEventListener::HandleEvent 
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp, line 182] 
nsXBLPrototypeHandler::ExecuteHandler 
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLPrototypeHandler.cpp, line 
448] 
DoKey [d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLKeyHandler.cpp, line 
108] 
nsXBLKeyHandler::KeyPress 
[d:\builds\seamonkey\mozilla\content\xbl\src\nsXBLKeyHandler.cpp, line 124] 
nsEventListenerManager::HandleEvent 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventListenerManager.cpp, line 
1654] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3461] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3442] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\xul\content\src\nsXULElement.cpp, line 
3442] 
nsGenericElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\base\src\nsGenericElement.cpp, line 1644] 
nsHTMLInputElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLInputElement.cpp, 
line 1411] 
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6105] 
PresShell::HandleEvent 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 6028] 
nsViewManager::HandleEvent 
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 2030] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 306] 
nsViewManager::DispatchEvent 
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 1887] 
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 83] 
nsWindow::DispatchEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 869] 
0x00adf010 
nsWindow::DispatchKeyEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 2660] 
nsWindow::OnChar [d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, 
line 2810] 
nsWindow::ProcessMessage 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3459] 
nsWindow::WindowProc 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 1131] 
USER32.dll + 0x1820 (0x77e71820) 
0x011c0001

Comment 1

[rubydoo123]

Attachment: stacktrace

Comment 2

[rubydoo123]

giving to av for review

Comment 3

[av (gone)]

IE cannot display the file, it goes to some sort of a loop, you can see that by
blinking busy cursor. With Mozilla I see different stacks on crash, some of them
indicating recursive function calls and obvious stack overflow.

Comment 4

[serge (gone)]

Attachment: patch proposal

the crash happens on dereferencing null prt
using NS_ENSURE_SUCCESS(GetPresShell(getter_AddRefs(shell)), NS_ERROR_FAILURE);

when GetPresShell()  can return NS_OK & shell==0 is not the right thing do use
the patch is obvious

Comment 5

[serge (gone)]

how I've  got the crash:
1. goto http://access.adobe.com/browser/netscape/namedest/despair.pdf#stupidity
2. delete #stupidity <ENTER>
3. click BACK.

Comment 6

[av (gone)]

Attachment: stack trace I see on the branch

With the patch I still crash with this stack trace (the same as without the
patch). In fact, I never saw the originally reported call stack, although I can
consistently reproduce the crash.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.0rc3) Gecko/20020520

Comment 7

[serge (gone)]

hmm, nsPluginByteRangeStreamListener::OnStartRequest()
in this case its mine:( 
investigating...

Comment 8

[av (gone)]

The file looks a bit corrupt but still shows fine if saved locally.

Comment 9

[serge (gone)]

oh, well, we cannot keep strong reference on nsPluginStreamListenerPeer in 
nsPluginByteRangeStreamListener::nsPluginByteRangeStreamListener(nsPluginStreamL
istenerPeer* 
aPluginStreamListenerPeer)
{
  NS_INIT_REFCNT();
  // don't addref
  mFinalPluginStreamListener = aPluginStreamListenerPeer;
}
due to asynchronous nature of  necko channels, 
if we fire several channel->AsyncOpen() from nsPluginStreamInfo::RequestRead() 
and then kill the page these channels could be still alive and call as with 
On[Start,DataA,Stop] but at that point
nsPluginByteRangeStreamListener.mFinalPluginStreamListener points to deleted 
object.
I'm going to add nsSupportsWeakReference to class nsPluginStreamListenerPeer

Comment 10

[serge (gone)]

Attachment: patch v1, add nsSupportsWeakReference to class nsPluginStreamListenerPeer

it allows us to handle delayed response on byte range requests from http
channel properly. 
Please review.

Comment 11

[serge (gone)]

BTW: with this patch v1 (attachment 84553 [details] [diff] [review]) I'm  crashing with original reported 
stack trace, I have to apply the first patch (attachment 84463 [details] [diff] [review]) to prevent 
dereferencing null prt in nsDocShell::GetRootScrollableView()
So we have 2 bugs here, shell split it?

Comment 12

[av (gone)]

Comment on attachment 84553 [details] [diff] [review]
patch v1, add nsSupportsWeakReference to class nsPluginStreamListenerPeer

I tried this patch on my branch build and it definitely fixes the crash I saw
without it. The pdf file itself is not displayed though, but this is probably a
different issue.

Comment 13

[av (gone)]

Comment on attachment 84553 [details] [diff] [review]
patch v1, add nsSupportsWeakReference to class nsPluginStreamListenerPeer

r=av

Comment 14

[av (gone)]

Comment on attachment 84553 [details] [diff] [review]
patch v1, add nsSupportsWeakReference to class nsPluginStreamListenerPeer

Just a small nit, would it be possible to use some less generic variable name
here:

+  nsPluginByteRangeStreamListener(nsIWeakReference* aWeakPtr);

Comment 15

[rubydoo123]

Darin: can you please review this for us, lots of network stuff touched

Comment 16

[Darin Fisher]

Comment on attachment 84553 [details] [diff] [review]
patch v1, add nsSupportsWeakReference to class nsPluginStreamListenerPeer

>Index: nsPluginHostImpl.cpp

>+  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(request));
>+  if (!httpChannel) {
>+    return NS_ERROR_FAILURE;
>+  }
>+     
>+  PRUint32 responseCode = 0;
>+  rv = httpChannel->GetResponseStatus(&responseCode);
>+  if (NS_FAILED(rv) || responseCode != 200) {
>+    return NS_ERROR_FAILURE;
>   }
> 
>   // if server cannot continue with byte range (206 status) and sending us whole object (200 status)
>   // reset this seekable stream & try serve it to plugin instance as a file

so this confuses me a little.  it looks like you are going to fail if you get
a 200 response, but what if you get a 206... is that really a failure?	is it
that we shouldn't be reaching this code if we're getting a 206?

otherwise, this patch seems fine to me.  sr=darin

Comment 17

[serge (gone)]

yes, we do not suppose to reach this code on 206,
unless nsMultiMixedConv::OnStartRequest() failed by some reason, in which case 
we failed too.

Comment 18

[serge (gone)]

Comment on attachment 84553 [details] [diff] [review]
patch v1, add nsSupportsWeakReference to class nsPluginStreamListenerPeer

this patch checked in on the trunk
mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp
new revision: 1.386; previous revision: 1.385

Comment 19

[serge (gone)]

for crash in nsDocShell::GetRootScrollableView I've opend bug 147256.
marked this as fix-trunk.

Comment 20

[serge (gone)]

adding adt1.0.0, nominating for the branch

Comment 21

[scottputterman]

shrir, could you verify this on the trunk?

Comment 22

[shrirang khanzode]

oh no, this still crashes on trunk 0528 on NT. What I did was this :

1 load http://access.adobe.com/browser/netscape/namedest/despair.pdf#stupidity
2 wait for a long time (blank page appears after acrobat splash screen)...   
'connecting access.adobe.com' ...
3 clicked on STOP and removed '#stupidity' and hit ENTER

BOOM !
http://climate.netscape.com/reports/incidenttemplate.cfm?bbid=6796148

It's the same trace...reopening

Comment 23

[serge (gone)]

Shrirang, the stack trace from TB 6796148 belongs to bug 147256, which I opened 
(comment #19) to fix the crash you are seeing. To test the fix for the patch 
(attachment 84553 [details] [diff] [review]) you have to access big enough pdf file over http://,
e.g.  http://www.gl.iit.edu/wadc/history/Roswell/Report/pt03a.pdf  
click as fast as you can on different bookmarks or thumbnails in acrobat window, 
that hit reload or back button.
w/o this check in (try branch build) you'll get a stack trace av mentioned 
in attachment 84499 [details]. 
Sorry for the mess.

Comment 24

[shrirang khanzode]

oops, I was clearly sleeping while verifying this one.
Thanks for the clarification, Serge!
I tested with the new steps, could not reproduce a crash anymore. 

Comment 25

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
Drivers approval. 

Comment 26

[Judson Valeski]

please checkin to the 1.0.1 branch ASAP. once there, remove the mozilla1.0.1+
keyword and add the fixed1.0.1 keyword.

Comment 27

[serge (gone)]

checked in MOZILLA_1_0_BRANCH
mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v  <--  nsPluginHostImpl.cpp
new revision: 1.372.2.15; previous revision: 1.372.2.14

Comment 28

[shrirang khanzode]

verified on 1.0.0 brnch build 0603 that this works just fine.