Closed Bug 131725 Opened 22 years ago Closed 22 years ago

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

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: abs, Assigned: jst)

References

(
URL
)

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(5 files)

WinNT stack trace
5.04 KB, text/plain
Details
this is a band aid
505 bytes, patch
Details | Diff | Splinter Review
Patch checks for nsnull
1.26 KB, patch
Brade
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
Fix crash w/o throwing an exception
8.29 KB, patch
Details | Diff | Splinter Review
diff -w of the above changes
7.50 KB, patch
adamlock
: review+
darin.moz
: superreview+
asa
: approval+
Details | Diff | Splinter Review 
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
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)
Severity: major → critical
Keywords: crash
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
Attached file WinNT stack trace — 

    
    

    
  
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

    
  
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]

    
    

    
  
###!!! 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]


    
    

    
  
fwiw i'm on freebsd

    
    

    
  
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.

    
    

    
  


  
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.

    
    

    
  
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.

    
    

    
  


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

    
    

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

sr=darin

        Attachment #74969 -
        Flags: superreview+

    
    

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

r=brade

        Attachment #74969 -
        Flags: review+

    
    

    
  
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+

    
    

    
  
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+

    
    

    
  
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 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 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+

    
    

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

    
    

    
  
Reassigning.

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

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

    
    

    
  
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+

    
    

    
  
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: