Closed Bug 395581 Opened 16 years ago Closed 15 years ago

Crash with Firebug [@ nsHttpTransaction::Close]

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: asrail, Assigned: Biesinger)

Details

(Keywords: crash, Whiteboard: [has-patch] [firebug-p5])

Crash Data

Attachments

(2 files, 1 obsolete file)

Stack trace
1.71 KB, text/plain
Details
patch
1.19 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
shaver
: approval1.9+
Details | Diff | Splinter Review
Attached file Stack trace — 
It crashes on:
###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/xpcom/nsCOMPtr.h, line 857

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1082132800 (LWP 2349)]
0x00002b45fb3ee484 in nsHttpTransaction::Close (this=0x2aaab4fc6520, 
    reason=2152924148)
    at /home/asrail/mozbuilds/mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp:622
622         mPipeOut->CloseWithStatus(reason);

When mPipeOut is a null pointer.


Steps to reproduce:
1 - Download Firebug from http://fbug.googlecode.com/svn/branches/firebug1.1 using SVN
1.a - For builds newer than Sep 04, you should edit install.rdf.tpl.xml and remove the updateURL, otherwise it won't install
2 - Build the package running 'ant'
3 - Install the xpi package located in dist/
4 - Load gmail
5 - Enable Firebug
6 - Login to gmail
Attached patch Proposed patch (obsolete) — — Splinter Review 
I'll ask reviews later.
Assignee: nobody → asrail
Status: NEW → ASSIGNED
Attachment #280249 - Flags: review?(bzbarsky)
You can change the 3 first steps for:
1 - Download 1.1b from: http://fireclipse.xucia.com/#Downloads
2 - Extract it
2.a -  For builds newer than Sep 04, you should, unzip, edit install.rdf,
remove the updateURL, and zip it again as xpi, otherwise it won't install
3 - Install the xpi package


Here, Firefox keeps impeaching the page for loading... I've to disable Firebug, load the page, then enable Firebug and click on "standard with chat" or "standard without chat".

The system is a:
Linux 2.6.20.3 #1 SMP PREEMPT x86_64 GNU/Linux

and I've tried with my own SeaMonkey trunk builds and Firefox 32 bits builds from Mozilla.org (Sep, 10th).
Attachment #280249 - Flags: review?(bzbarsky) → review?(cbiesinger)
Seeing this on OS X as well. Marking All/All and requesting blocking1.9.
Severity: normal → critical
Flags: blocking1.9?
Keywords: crash
OS: Linux → All
Hardware: PC → All
A complete call stack for 3a8:
http://crash-stats.mozilla.com/report/index/2da6f4a1-7168-11dc-9e58-001a4bd43ed6?date=2007-10-03-04

There is no Javascript on the stack; I don't know how to workaround this in Firebug.
Regarding comment 2 and comment 0, with Firebug 1.1b2 you don't need to edit anything, just install it.

I believe this bug is related to threads and is showing up with Firebug due to its interference with the network workflow.

And if you set Firebug->Net->Options->DisableNetworkMonitor checked, FF3a8 does not crash.
it will take me a while to review this patch, I need to figure out why mPipeOut is null here, and whether not calling Close is correct

(In reply to comment #7)
> it will take me a while to review this patch, I need to figure out why mPipeOut
> is null here, and whether not calling Close is correct

I don't know how much time it will save you, but...
If I revert the patch and set a breakpoint on nsHttpTransaction::Close it won't crash.

Sometimes the address of mPipeOut and mPipeIn becomes some strange number, as:
0x2aaab413afa8 (not likely to be a valid pointer).

Since a short amount of time matters here, I believe it may be related to threads.
Flags: blocking1.9? → blocking1.9+
The issue happens with POST XmlHttpRequests (simpler testcase than gmail: http://www.openjs.com/scripts/examples/ajax_using_post.php).

In the "http-on-modify-request" observer, Firebug is closing the request's upload stream. Here's a JS backtrace when using a breakpoint in nsStringInputStream::Close():

0 [native frame]
1 anonymous(stream = ?unknown?, charset = "UTF-8") ["chrome://firebug/content/lib.js":2508]
    sis = [xpconnect wrapped nsIBinaryInputStream @ 0x8e60098 (native @ 0x92647b8)]
    segments = lorem=ipsum&name=binny
    count = 0
    text = undefined
    this = function XPCOMUtils() {
}
2 getPostText(request = [object XMLHttpRequest @ 0x93cf148 (native @ 0xb22e2c88)], context = [object Object]) ["chrome://firebug/content/spy.js":549]
    charset = "UTF-8"
    text = undefined
    ss = undefined
    this = [object ChromeWindow @ 0xb66d87c8 (native @ 0x898379c)]
3 requestStarted(request = [object XMLHttpRequest @ 0x93cf148 (native @ 0xb22e2c88)], context = [object Object], method = "POST", url = "http://www.openjs.com/scripts/jx/data.php") ["chrome://firebug/content/spy.js":351]
    spy = [object Object]
    this = [object ChromeWindow @ 0xb66d87c8 (native @ 0x898379c)]
4 anonymous(request = [xpconnect wrapped (nsISupports, nsIHttpChannel, nsIChannel, nsIUploadChannel) @ 0x9306d90 (native @ 0x93a96c8)], topic = "http-on-modify-request", data = null) ["chrome://firebug/content/spy.js":45]
    xhrRequest = [object XMLHttpRequest @ 0x93cf148 (native @ 0xb22e2c88)]
    win = [object XPCCrossOriginWrapper [object Window @ 0x8c6f748 (native @ 0x8c726a4)]]
    i = 0
    this = [object Object]
5 [native frame]
6 postMethod() ["http://www.openjs.com/scripts/examples/ajax_using_post.php":53]
7 onclick(event = [object MouseEvent @ 0x930b660 (native @ 0x9182cb0)]) ["http://www.openjs.com/scripts/examples/ajax_using_post.php":1]
    this = [object HTMLInputElement @ 0xb6720fb0 (native @ 0xb2203450)]


Around "chrome://firebug/content/lib.js":2508, there is:

this.readFromStream = function(stream, charset)
{
    try
    {
        var sis = this.CCSV("@mozilla.org/binaryinputstream;1", "nsIBinaryInputStream");
        sis.setInputStream(stream);

        var segments = [];
        for (var count = stream.available(); count; count = stream.available())
            segments.push(sis.readBytes(count));

        sis.close();
        var text = segments.join("");
        return this.convertToUnicode(text, charset);
     }
     catch(exc)
     {
     }
};

Because the upload stream is closed, nsStringInputStream::Available() fails, which make nsIMultiplexInputStream::Available() fail at http://mxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpTransaction.cpp#281, and so the mPipe* instance variables are not initialized.

Thus, later when nsHttpTransaction::Close() is called it crashes.


What do you think is best to do for fixing this?
John, do you know if there were changes related to the net.js/spy.js in Firebug version 1.1 in comparison to version 1.0?
I'm wondering why this isn't an issue on branch.
Yes, there were several changes in net.js and spy.js between 1.05 (the getfirebug.com version) and 1.1. The biggest change was to hook http-on-modify-request rather than overwrite XMLHttpRequest methods. 
Apparently it works on branch because before bug 318193, closing the nsStringInputStream didn't clear the stream content.

The workaround is not to close the stream. I've attached a patch on the Firebug ticket: http://code.google.com/p/fbug/issues/detail?id=293

Regarding this bug, we could check that the upload channel is not closed after the "http-on-modify-request" observer has been called, but the damage has already been made then (the upload channel content is gone).
(In reply to comment #12)
> Apparently it works on branch because before bug 318193, closing the
> nsStringInputStream didn't clear the stream content.
> 
> The workaround is not to close the stream. I've attached a patch on the Firebug
> ticket: http://code.google.com/p/fbug/issues/detail?id=293
> 
> Regarding this bug, we could check that the upload channel is not closed after
> the "http-on-modify-request" observer has been called, but the damage has
> already been made then (the upload channel content is gone).
> 

You know this code a lot more than me... do you want to take this bug?
If you wanna, go ahead and take it.



We'll implement the workaround for Firebug 1.2, thanks.
Priority: -- → P3
Whiteboard: [has-patch]
Can someone estimate whether this problem will be solved before FF3.0 ships? we may need to issue a workaround in Firebug before 1.2 if this is on fixed in FF3.
Thanks.
(In reply to comment #15)
> Can someone estimate whether this problem will be solved before FF3.0 ships? we
> may need to issue a workaround in Firebug before 1.2 if this is on fixed in
> FF3.

John, there are two separate facts related to this bug (below, Gecko 1.9 is Firefox 3, and Gecko 1.8 Firefox 2):

1) In Gecko 1.9, things are handled differently as 1.8 (comment 12), which means that Observers of http-on-modify-request shouldn't close the upload stream, otherwise the data is lost.

2) Gecko 1.9 shouldn't crash if consumers do not respect the constraints of 1)

For 1), this means that Firebug needs to be changed. The patch in http://code.google.com/p/fbug/issues/detail?id=293 one way to fix this.

I don't think Gecko can do much for 1), but something could be done here for 2).

The change on http://code.google.com/p/fbug/issues/detail?id=293 should be backward compatible on Gecko 1.8 (it would be good to check if this doesn't introduce leaks).



Ok, thanks.  However I don't understand the fix.  In its  http-on-modify-request observer Firebug only closes the stream opened in the observer:
---
        var sis = this.CCSV("@mozilla.org/binaryinputstream;1", "nsIBinaryInputStream");
        sis.setInputStream(stream);

        var segments = [];
        for (var count = stream.available(); count; count = stream.available())
            segments.push(sis.readBytes(count));

        sis.close();
---
The patch removes the call to close().  I can't find any docs on these stream functions that explain what "close()" does.  Are these objects like Java streams where they essentially pipes so "close()" ripples back and closes the file? If the file is closed by another means is there any reason to close the nsIBinaryInputStream?  
(In reply to comment #7)
> it will take me a while to review this patch, I need to figure out why mPipeOut
> is null here, and whether not calling Close is correct

Sylvain Pasche made some interesting comments explaining this, as you can see in here.

Any advances on review?
"NS_ENSURE_STATE" would be better?

(In reply to comment #17)
> The patch removes the call to close().  I can't find any docs on these stream
> functions that explain what "close()" does.  Are these objects like Java
> streams where they essentially pipes so "close()" ripples back and closes the
> file? If the file is closed by another means is there any reason to close the
> nsIBinaryInputStream?  

The close() method is documented on devmo:
http://developer.mozilla.org/en/docs/nsIInputStream

There are more information in the .idl file
(http://developer.mozilla.org/en/docs/nsIInputStream):

 Close the stream.  This method causes subsequent calls to Read and
 ReadSegments to return 0 bytes read to indicate end-of-file.  Any
 subsequent calls to Available should throw NS_BASE_STREAM_CLOSED.

The reason not to close it at this stage is that this stream is the same one
that will be used to POST the data after the observer has been called. So once
closed, the data is lost and won't be available for posting (and also makes
Firefox crash).


Asrail,
I think I didn't check last time, but with your patch applied and a FF3 compatible Firebug with XmlHTTPRequest logging active is the POST data sent to the server?

For testing, you could try for instance http://www.brainjar.com/dhtml/ajax/demo3.html
(In reply to comment #20)
> Asrail,
> I think I didn't check last time, but with your patch applied and a FF3
> compatible Firebug with XmlHTTPRequest logging active is the POST data sent to
> the server?
> 
> For testing, you could try for instance
> http://www.brainjar.com/dhtml/ajax/demo3.html

On the site you posted it still working, but some things on gmail stop working.
And on http://www.openjs.com/scripts/examples/ajax_using_post.php, I got:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80470002: file /home/asrail/mozbuilds/mozilla/xpcom/io/nsMultiplexInputStream.cpp, line 180
WARNING: NS_ENSURE_TRUE(listener) failed: file /home/asrail/mozbuilds/mozilla/content/base/src/nsXMLHttpRequest.cpp, line 529
WARNING: NS_ENSURE_TRUE(listener) failed: file /home/asrail/mozbuilds/mozilla/content/base/src/nsXMLHttpRequest.cpp, line 529

and it won't work.


Since Firebug is closing the stream, I believe it'll have to be fixed on their side, anyway.
Of course, avoiding a leak on Gecko 1.8.

But we should avoid a crash for Gecko.

Just for reference, 0x80470002 is NS_BASE_STREAM_CLOSED.
Asrail, thanks for testing. That's confirming the theory above (on the site I posted I should have specified that you need to click on the "Lookup ZIP Codes" to test the Ajax posting, but if it fails elsewhere that shows the issue).
(In reply to comment #19)
> The reason not to close it at this stage is that this stream is the same one
> that will be used to POST the data after the observer has been called. So once
> closed, the data is lost and won't be available for posting (and also makes
> Firefox crash).

Sorry to beat on this but if I put a fix on Firebug 1.1.0 at this point I want
to be pretty careful. You say "this stream is the same one", but the object
that is closed in the Firebug code is not the object that is subsequently used
for post.  The sis.close() is called and the object "stream" is used for post.
They are connected to with 
  sis.setInputStream(stream);
Does the sis.close() cause stream.close()? If yes then I understand the
problem. Is sis.close() needed if stream.close() is called elsewhere? If no,
then I understand the fix.  
Thanks,
John.

(In reply to comment #24)
> Does the sis.close() cause stream.close()?

Sure, it will just proxy the close() call to the wrapped stream:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/io/nsBinaryStream.cpp&rev=1.32&mark=457-471#457
(In reply to comment #23)
> Asrail, thanks for testing. That's confirming the theory above (on the site I
> posted I should have specified that you need to click on the "Lookup ZIP Codes"
> to test the Ajax posting, but if it fails elsewhere that shows the issue).

I don't have any combination of city and state on US to test that function, so the other link is easier to test it.




Ok thanks for all of the help. I verified that I crashed on
 http://www.brainjar.com/dhtml/ajax/demo3.html
then removed the close() calls and verified that the same URL does not crash.
Fix is on 
https://fbug.googlecode.com/svn/branches/firebug1.2
https://fbug.googlecode.com/svn/branches/firebug1.1
https://fbug.googlecode.com/svn/branches/eval
and will be in firebug-1.1.0b10.

(In reply to comment #27)
> Ok thanks for all of the help. I verified that I crashed on
>  http://www.brainjar.com/dhtml/ajax/demo3.html
> then removed the close() calls and verified that the same URL does not crash.
> Fix is on 
> https://fbug.googlecode.com/svn/branches/firebug1.2
> https://fbug.googlecode.com/svn/branches/firebug1.1
> https://fbug.googlecode.com/svn/branches/eval
> and will be in firebug-1.1.0b10.

So this is now WFM/INVALID? Please renom if it needs to block.
Flags: tracking1.9+
From the Firebug perspective there is no need to block. Our code was wrong which tripped the crash; now our code is fixed and we don't crash.
Extensions can crash the browser if they behave incorrectly in the http-on-modify-request listener, which was the case with Firebug. I think this bug could still exist as an enhancement request to avoid crashing under those conditions. See comment 16 point 2).
Biesi, this looks like a simple one-liner patch. Can you review, just to get it off the radar?
Whiteboard: [has-patch] → [has-patch] [firebug-p5]
Flags: wanted-next+
That is not a simple patch.

Why is the HttpConnMgr involved (as shown in the attached stack) when nsHttpTransaction::Init failed (as per comment 9)?
Comment on attachment 280249 [details] [diff] [review]
Proposed patch

So to answer comment 9, the fix is to not get Close called when Init failed.
Attachment #280249 - Flags: review?(cbiesinger) → review-
http://demo.zarafa.com/ crashes with 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030317 Firefox/3.0b4
with
http://www.getfirebug.com/releases/firebug/1.1/firebug-1.1.0b12.xpi
figured it out

We call OnStartRequest on the listener, the listener calls cancel on the httpchannel, the implementation of Cancel calls httpconnectionmanager->CancelTransaction because mTransaction is not null

will attach a patch in a second, although it's untested.
Attached patch patch — — Splinter Review 
Unfortunately I couldn't test this yet because I don't currently have a build. If someone else could that'd be great, otherwise I'll test this tomorrow night.
Assignee: asrail → cbiesinger
Attachment #280249 - Attachment is obsolete: true
Attachment #308748 - Flags: superreview?(bzbarsky)
Attachment #308748 - Flags: review?(bzbarsky)
Priority: P3 → --
Comment on attachment 308748 [details] [diff] [review]
patch

This seems like an eminently reasonable thing to me!
Attachment #308748 - Flags: superreview?(bzbarsky)
Attachment #308748 - Flags: superreview+
Attachment #308748 - Flags: review?(bzbarsky)
Attachment #308748 - Flags: review+
Comment on attachment 308748 [details] [diff] [review]
patch

a=shaver, with glee and mad props.
Attachment #308748 - Flags: approval1.9? → approval1.9+
I tested this and it does fix the crash. There's an assertion from nsPipeInputStream::Seek, which I'll investigate later, but that assertion should probably be removed anyway.
Checking in nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.328; previous revision: 1.327
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
The assertion I mentioned is now bug 422537 and points, imo, to a pretty severe problem...
this is not really interesting as bug 422537 will fix that; but even after this patch the HTTP request was not actually sent, because if a pipe's input stream is at EOF it behaves like it was closed, so Available() still failed. (assumes that the output end was closed of course, but that's the case here)
Crash Signature: [@ nsHttpTransaction::Close]
You need to log in before you can comment on or make changes to this bug.