Javascript and named frames crash bug; M099 [@ LocationImpl::GetProtocol]

VERIFIED FIXED in mozilla1.0

Status

()

Core
Document Navigation
--
critical
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: David Brownlee, Assigned: jst)

Tracking

({crash, topcrash})

Trunk
mozilla1.0
x86
All
crash, topcrash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
We have a dynamo based publishing system which uses frames and javascript.
Mozilla   has worked fine up to and including 0.9.8. New with 0.9.9 it crashes
mozilla on login. This makes mozilla unususable for us.

I have distilled down a javascript/frames fragment which reproduces the problem.
Its available at http://www.mono.org/abs/bang/, or as a tarfile of the frames at
http://www.mono.org/abs/bang/mozilla_bang.tar
Confirmed, build 2002-03-17-21 on Linux SuSE 7.3
Talkback id: TB4176863G
Severity: major → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
I see this crash with 0.9.9 on Linux.  I do _not_ see a crash with the
2002-03-18-06 trunk nightly on Linux.
Severity: critical → major
stephend, could you get the stack?  TB4176863G

Comment 4

17 years ago
Here it is:

Incident ID       4176863
Stack Signature   LocationImpl::GetProtocol() 310c2cd2
Trigger Time      2002-03-18 09:25:03
Email Address     mats@symsoft.se
URL visited       http://www.mono.org/abs/bang/
Build ID          2002031721
Product ID        MozillaTrunk
Platform
Operating System  LinuxIntel
Module
Trigger Reason    SIGSEGV: Segmentation Fault: (signal 11)
User Comments     Bugzilla bug 131725 
Stack Trace

LocationImpl::GetProtocol() 
XPTC_InvokeByIndex() 
XPCWrappedNative::CallMethod() 
XPC_WN_GetterSetter() 
js_Invoke() 
js_InternalInvoke() 
js_GetProperty() 
js_Interpret() 
js_Execute() 
JS_EvaluateUCScriptForPrincipals() 
nsJSContext::EvaluateString() 
nsScriptLoader::EvaluateScript() 
nsScriptLoader::ProcessRequest() 
nsScriptLoader::ProcessScriptElement() 
nsHTMLScriptElement::SetDocument() 
nsGenericHTMLContainerElement::AppendChildTo() 
HTMLContentSink::ProcessSCRIPTTag() 
HTMLContentSink::AddLeaf() 
CNavDTD::AddLeaf() 
CNavDTD::HandleScriptToken() 
CNavDTD::OpenContainer() 
CNavDTD::HandleDefaultStartToken() 
CNavDTD::HandleStartToken() 
CNavDTD::HandleToken() 
CNavDTD::BuildModel() 
nsParser::BuildModel() 
nsParser::ResumeParse() 
nsParser::OnDataAvailable() 
nsDocumentOpenInfo::OnDataAvailable() 
nsStreamListenerTee::OnDataAvailable() 
nsHttpChannel::OnDataAvailable() 
nsOnDataAvailableEvent::HandleEvent() 
nsARequestObserverEvent::HandlePLEvent() 
PL_HandleEvent() 
PL_ProcessPendingEvents() 
nsEventQueueImpl::ProcessPendingEvents() 
event_processor_callback() 
our_gdk_io_invoke() 
libglib-1.2.so.0 + 0xf3b0 (0x403903b0) 
libglib-1.2.so.0 + 0x10c46 (0x40391c46) 
libglib-1.2.so.0 + 0x11273 (0x40392273) 
libglib-1.2.so.0 + 0x1143c (0x4039243c) 
libgtk-1.2.so.0 + 0x9276c (0x402aa76c) 
nsAppShell::Run() 
nsAppShellService::Run() 
main1() 
main() 
libc.so.6 + 0x1d7ee (0x404df7ee) 

Updated

17 years ago
Severity: major → critical
Keywords: crash

Comment 5

17 years ago
I can reproduce this crash simply by loading the given URL in my
debug WinNT build 2002-03-14. Will attach stack trace below.

OS: NetBSD ---> All
OS: NetBSD → All

Comment 6

17 years ago
Created attachment 74772 [details]
WinNT stack trace

Comment 7

17 years ago
Not sure what this stack trace indicates. Provisionally assigning to
XPConnect for help; cc'ing jband. This is an easy crash to reproduce.

Is this JS Engine or XPConnect-related, do you think?
Here is the parent HTML page:

<HTML>
<FRAMESET rows="*,*">
<FRAME src="broken_js.html">
<FRAME name=STIM_Preview src="blank.html">
</FRAMESET>
</HTML>


Here is broken_js.html:


<HTML>
<BODY>
<SCRIPT language=JavaScript1.1>

parent.top.STIM_Preview.location.href =                                                                                                          
                                   parent.top.STIM_Preview.location.protocol
                                   + "//"
                                   + parent.top.STIM_Preview.location.host
                                   + parent.top.STIM_Preview.location.pathname;

</SCRIPT>
</BODY>
</HTML>
Assignee: rogerl → dbradley
Component: JavaScript Engine → XPConnect

Updated

17 years ago
Assignee: dbradley → adamlock
Component: XPConnect → Embedding: Docshell
QA Contact: pschwartau → adamlock
Summary: Javascript and named frames crash bug → Javascript and named frames crash bug [@LocationImpl::GetProtocol]

Comment 8

17 years ago
###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/xpcom/nsCOMPtr.h, line 650

this assert comes from uri (surprise).

Going backwards, i jumped to 
645       result = GetURI(getter_AddRefs(uri)); /*where uri is set*/
647       if (NS_SUCCEEDED(result)) {
648         nsCAutoString protocol;
649
650         result = uri->GetScheme(protocol); /*where the assert is*/

And this is where i ended up:
2482    nsDocShell::GetCurrentURI(nsIURI ** aURI)
2483    {
2484        NS_ENSURE_ARG_POINTER(aURI);
2485
2486        *aURI = mCurrentURI;
2487        NS_IF_ADDREF(*aURI);
2488
2489        return NS_OK;
2490    }
2491
(gdb) p mCurrentURI
$10 = {mRawPtr = 0x0}
(gdb) where
#0  nsDocShell::GetCurrentURI (this=0x9e48600, aURI=0xbfbfd0d0) at /home/timeless/mozilla/docshell/base/nsDocShell.cpp:2487
#1  0x29857d39 in LocationImpl::GetURI (this=0x9ea5b00, aURI=0xbfbfd0d0) at /home/timeless/mozilla/dom/src/base/nsLocation.cpp:242
#2  0x29859c42 in LocationImpl::GetProtocol (this=0x9ea5b00, aProtocol=@0xbfbfd2d4) at /home/timeless/mozilla/dom/src/base/nsLocation.cpp:645
#3  0x282fa68c in XPTC_InvokeByIndex (that=0x9ea5b00, methodIndex=15, paramCount=1, params=0xbfbfd368)
    at /home/timeless/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:153

I don't think we should return NS_OK if the getter fails [nor did the  LocationImpl coders]

Comment 9

17 years ago
fwiw i'm on freebsd

Comment 10

17 years ago
the solution to this bug depends on the how nsIDocShell is meant to work... if
it is OK for currentURI to return NULL, then LocationImpl should work around
this behavior, possibly returning an error from GetURI when the URI is NULL.

Comment 11

17 years ago
Created attachment 74839 [details] [diff] [review]
this is a band aid

taking this pill allows me to load the page, this is what venkman gives me:
function (null)() in <http://www.mono.org/abs/bang/broken_js.html> line 4
Continuing from thrown exception.
002: <BODY>
003: <SCRIPT language=JavaScript1.1>
004: parent.top.STIM_Preview.location.href =
parent.top.STIM_Preview.location.protocol + "//
005: + parent.top.STIM_Preview.location.host
006: + parent.top.STIM_Preview.location.pathname;

presumably it's not really what we want for final. i wonder if the actual
problem is a race condition where the other frame hasn't loaded yet so location
isn't available for reading.

Comment 12

17 years ago
currentURI is a readonly attribute on nsIWebNavigation that *can* be nsnull. For
example, a newly created docshell has no current uri. 

The interface documentation could be a little clearer but I see nothing wrong
with the current implementation.

Therefore callers should test the value is non-null in addition or instead of
looking at the nsresult if they intend to use it. I will do a simple patch for
nsLocation.cpp and make the documentation more clear.

Comment 13

17 years ago
Created attachment 74969 [details] [diff] [review]
Patch checks for nsnull

Patch adds a check for nsnull inside LocationImpl::GetURI. A null value makes
this method return NS_ERROR_FAILURE.

Comment 14

17 years ago
Comment on attachment 74969 [details] [diff] [review]
Patch checks for nsnull

sr=darin
Attachment #74969 - Flags: superreview+

Comment 15

17 years ago
Comment on attachment 74969 [details] [diff] [review]
Patch checks for nsnull

r=brade
Attachment #74969 - Flags: review+
(Assignee)

Comment 16

17 years ago
Comment on attachment 74969 [details] [diff] [review]
Patch checks for nsnull

This doesn't seem like the right thing to do to me, just because there's no
current URI doesn't mean that we should throw an exception in the callers face,
in stead we should just return no URI, or default to about:blank or something
in the DOM code.

I'm working on a patch for this...
Attachment #74969 - Flags: needs-work+
(Assignee)

Comment 17

17 years ago
Created attachment 75010 [details] [diff] [review]
Fix crash w/o throwing an exception
(Assignee)

Comment 18

17 years ago
Created attachment 75011 [details] [diff] [review]
diff -w of the above changes

Comment 19

17 years ago
Comment on attachment 75011 [details] [diff] [review]
diff -w of the above changes

look great, sr=darin

though we should also be sure to land the documentation change to
nsIDocShell::currentURI.
Attachment #75011 - Flags: superreview+

Comment 20

17 years ago
topcrash on M099; added M099 to summary for tracking
Keywords: topcrash
Summary: Javascript and named frames crash bug [@LocationImpl::GetProtocol] → Javascript and named frames crash bug; M099 [@LocationImpl::GetProtocol]

Comment 21

17 years ago
Comment on attachment 75011 [details] [diff] [review]
diff -w of the above changes

r=adamlock

I am concerned that GetWritableURI will return NS_OK when it fails, but the
logic for handling the return value looks ok.

Comment 22

17 years ago
Comment on attachment 75011 [details] [diff] [review]
diff -w of the above changes

r=adamlock

I am concerned that GetWritableURI will return NS_OK when it fails, but the
logic for handling the return value looks ok.
Attachment #75011 - Flags: review+
(Assignee)

Comment 23

17 years ago
Adam, wanna land this one? If not, hand it over to me and I'll do it...

Comment 24

17 years ago
Reassigning.

I've asked Asa for the a= and I've checked in the tweak to the currentURI
comment nsIWebNavigation.idl
Assignee: adamlock → jst
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: mozilla1.0, nsbeta1
Target Milestone: --- → mozilla1.0

Comment 25

17 years ago
Comment on attachment 75011 [details] [diff] [review]
diff -w of the above changes

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #75011 - Flags: approval+
(Assignee)

Comment 26

17 years ago
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 27

17 years ago
Verifying fixed.  This last crashed with build 2002031910...no Talkback
incidents since then.
Status: RESOLVED → VERIFIED
Summary: Javascript and named frames crash bug; M099 [@LocationImpl::GetProtocol] → Javascript and named frames crash bug; M099 [@ LocationImpl::GetProtocol]
Crash Signature: [@ LocationImpl::GetProtocol]
You need to log in before you can comment on or make changes to this bug.