529041 - Crash in nsXMLHttpRequest::Send [@ xul.dll@0x438302] [@ xul.dll@0x42b6ff]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is currently the #2 topcrash for Firefox3.6b2

The stack basically starts out in 

nsDocument::DispatchContentLoadedEvents()  Line 3895

Firing the "DOMContentLoaded" event. Eventually we end up firing a JS-implemented event-listener which calls

nsXMLHttpRequest::Send(aBody=0x08ca2f00)  + 0x42c708 bytes

Seems like we're dying with a nullpointer dereference, but can't tell yet what exact code this is happening in.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The failing line is:

rv = uploadChannel->ExplicitSetUploadStream(postDataStream, contentType, -1, method, PR_FALSE);

with uploadChannel being null. Likely someone has replaced the http-channel implementation with one of their own, and hasn't implemented nsIUploadChannel2 on it (which isn't surprising since it's a new interface).

We could easily detect this and report to the error console. This would prevent the crash from happening, but would also prevent the code from working correctly.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking this blocking since it's a regression and a top-crasher.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Heavily correlated with McAfee SiteAdvisor, so they probably replace our http channel implementation.

http://people.mozilla.com/crash_analysis/20091116/20091116_Firefox_3.6b2-interesting-addons.txt

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bug 526224 is probably the same bug.

Comment 5

[timeless]

Attachment: patch

well, if you absolutely want this to work xmlhttprequest can be incestuous.

algorithm:
qi for nsIUploadChannel2, on failure rescue all your data from the existing channel and CI by ID the CID for our impl and then give it all the data you rescued and continue on with life.

note that it's illegal for anyone to recycle/shadow a CID, if anyone does we should blocklist them. As long as we play nice and try to use the contract first, I have no qualms about falling back to our working CID. To do this, xmlhttprequest should probably eagerly qi to nsIUploadChannel2.

Attached is a compiling implementation of the above algorithm. No testing has been done.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 412814 [details] [diff] [review]
patch

I don't think we'll want to do this. It duplicates too much code, and would make it impossible to override http-channels used by XHR. Even in cases where the channel does support nsIUploadChannel2, or when nsIUploadChannel would do just fine.

In any event it doesn't address the redirect code which suffers the same problem.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This patch makes us fall back to using nsIUploadChannel if nsIUploadChannel2 isn't available.

This results in a change in behavior, files with an unknown content type sent using XMLHttpRequest get a "Content-Type:application/octet-stream" header rather than no content-type header.

So we really do want people that implement a http protocol handler so that we can do the right thing. We should reach out to McAfee and get them to fix Site Advisor.

I put a console-service warning in the ioservice that detects if a returned http-channel can't be QIed to nsIUploadChannel2. However I was worried about performance impact if we warn every time a http channel is created, so we just warn the first time. We additionally warn every time XMLHttpRequest is used in order to reduce the likelyhood that developers don't see the warning.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 413010 [details] [diff] [review]
Patch to fix

+++ b/content/base/src/nsXMLHttpRequest.cpp

+        consoleService->LogStringMessage(NS_LITERAL_STRING("Http channel implementation doesn't support nsIUploadChannel2. An extension has supplied a non-functional http protocol handler. This will browser behavior and in future releases not work at all.").get());

Not localizable?

Even if you want to keep the message hardcoded, can you break it into 80-character lines?

Also s/browser/break/?

+++ b/netwerk/base/src/nsIOService.cpp
+                consoleService->LogStringMessage(NS_LITERAL_STRING("Http channel implementation doesn't support nsIUploadChannel2. An extension has supplied a non-functional http protocol handler. This will break browser behavior and in future releases not work at all.").get());

same comments as above (esp. line length)


Can you add a comment about why you're adding this warning, and mention this bug, and mention that it's temporary?

+        nsCOMPtr<nsIUploadChannel> uploadChannel;

Wouldn't it be simpler to just always initialize this?

Can you add a comment how not implementing uploadChannel2 can happen?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

You know... another way to fix this would be: keep using nsIUploadChannel; make up a special content type "x-internal/x-empty-content-type" and make the http channel interpret that as "send no content type". Slightly hacky, I admit.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I actually thought about that too. I don't really feel strongly either way. It does seem awfully hacky. And there's a risk that things would behave strangely since if someone overrides the http-channel they might do weird things with such a content-type.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Yeah, good point. I also don't feel strongly either way, so let's go with what we have here, I think.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fixed on trunk

http://hg.mozilla.org/mozilla-central/rev/9511efaadcea

We're string frozen so I can't make the message localizable at this point

Comment 14

[Brian Crowder]

+                consoleService->LogStringMessage(NS_LITERAL_STRING(
+                    "Http channel implementation doesn't support "
+                    "nsIUploadChannel2. An extension has supplied a "
+                    "non-functional http protocol handler. This will break "
+                    "behavior and in future releases not work at all.").get());
+            }

This is broken on Windows Mobile because the compiler seems to think the first string is wide and the rest are not (perhaps some fun with the way |L"foo" "bar| is handled?).

Comment 15

[Brendan Eich [:brendan]]

Yes, that's what NS_L() is for (dig that short macro name!).

/be

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fixed in 1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb4e5ea3ba69

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #13)
> We're string frozen so I can't make the message localizable at this point

Surely we're not string frozen on trunk...

Comment 18

[chris hofmann]

400-600 crashes per day in b2 and b3.  

what kind of testing is needed to verify the patch that landed on the branch in comment 16?

Comment 20

[chris hofmann]

re: comment 18

a lot of the user comments suggest they are doing a search on google or other search providers.  

Looking at what is going on in the discussion on the fix, and putting that together with the user comments on the crash, it sounds like an abstracted test case would emulate search suggestions where user starts typing and suggestions start appearing though the interaction via XMLHttpRequest.   

do we have such a test case, or a test case that suggests just pound heavily on a site or search box that has the search suggest feature.

does that sound right?

what other web app kinds of capabilities might exercise this code?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There's two approaches to testing this.

To test specifically what this addon does, i.e. register a protocol-handler and attempt to use XMLHttpRequest POST data to it.

Or simply run our existing mochi/unit/litmus tests with various addons installed.

IMHO we should do both.

Comment 22

[Ben Bucksch (:BenB)]

Code comment says:
    // Eventually we should remove this and simply require that http channels
    // implement the new interface.

This was 3 years ago. Time to remove this now?