233115 - xmethods.net testcase hangs

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[/\/\arcio Galli]

http://mgalli.com/wsdom/moz-crossportal/fs-2/

Comment 1

[Harshal Pradhan]

I started out trying to figure out why we have problems with our the
.Net-generated WSDL, but quickly ran into this bug. It basically
blocks any meaningful use of the mozilla web-services stack. 
(For me at least).

What is happening here is that nsWSAUtils::GetOfficialHostName()
never comes out of the eventloop that its spins up. This seems to be happening
because of the wierd interactions with the eventloop that nsXMLHttpRequest
spins. The event that nsDNSListener::OnLookupComplete() posts to the 
UI thread never gets processed. (Probably because it got posted to the wrong
event queue?). In any case, we are deeply nested in event queues at
this point because the wsdl code itself is using an XMLHttpRequest to fetch
the WSDL file, and I am having difficulty following what happens to that event.

There have been other reports of this in the newsgroups.

http://groups.google.com/groups?dq=&hl=en&lr=&ie=UTF-8&c2coff=1&threadm=c0l510%24ghg1%40ripley.netscape.com&prev=/groups%3Fhl%3Den%26lr%3D%26ie%3DUTF-8%26c2coff%3D1%26group%3Dnetscape.public.mozilla.xml

That seems to indicate that this regressed sometime after 1.5.
(Possibly a DNS rewrite regression?)

Comment 2

[Harshal Pradhan]

Attachment: Debug hack

This is just for reference. It possibly explains better what I was
talking about above. 

This makes the hang go away. It still doesn't make the testcase "work", but
I think thats another bug.

Comment 3

[Harshal Pradhan]

Attachment: Possible fix?

This patch attempts to use the dns service API to make sure that we get the
lookup complete notification on fhe right event queue. I am hoping it is
correct to assume that, this would mean that the listener would then no longer
have to bother about doing the event stuff itself. The proxy event magic should
do that for us. 

Also changed the eventloop to a for events before handling them so that it
doens't lock up CPU like it used to. 

This fixes the hang and seems to work for me.

Comment 4

[Harshal Pradhan]

Comment on attachment 142513 [details] [diff] [review]
Possible fix?

Darin, could you please have a look at this?

Comment 5

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

Comment on attachment 142513 [details] [diff] [review]
Possible fix?

+  if (NS_FAILED(rv))
+    return rv;

don't you need to pop the event queue when AsyncResolve fails?

Comment 6

[Darin Fisher]

Comment on attachment 142513 [details] [diff] [review]
Possible fix?

>Index: src/nsWSAUtils.cpp

>   nsCOMPtr<nsIEventQueue> eventQ;
>   rv = eventQService->PushThreadEventQueue(getter_AddRefs(eventQ));
>   
>   if (NS_FAILED(rv))
>+    return rv;
>+
>+  nsCOMPtr<nsIDNSRequest> dummy;
>+  rv = dns->AsyncResolve(host, PR_FALSE, listener, eventQ, getter_AddRefs(dummy));
>   
>+  if (NS_FAILED(rv))
>+    return rv;
>+    
>+  PLEvent *ev;
>+  while (!listener->mLookupFinished && NS_SUCCEEDED(rv)) {
>+     rv = eventQ->WaitForEvent(&ev);
>+     NS_ASSERTION(NS_SUCCEEDED(rv), "WaitForEvent failed...");
>+     if (NS_SUCCEEDED(rv)) {
>+       rv = eventQ->HandleEvent(ev);
>+       NS_ASSERTION(NS_SUCCEEDED(rv), "HandleEvent failed...");
>+     }
>+  }
>+
>   eventQService->PopThreadEventQueue(eventQ);

the early return after AsyncResolve is a major problem.  it would prevent the
event queue from being popped.	since you break out of the loop when not
NS_SUCCEEDED(rv), it seems like the early return is unnecessary.  maybe you can
just have the function return rv at the end (or something like that).

otherwise, this patch looks good.  the existing code WFM provided i only use
webservices back to the origin host.  anything cross-origin hangs as you have
described.

Comment 7

[Harshal Pradhan]

Attachment: Avoid early return

This is more or less the same. Just nests some stuff in an if() and returns rv
at the end.

Comment 8

[Harshal Pradhan]

Taking.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 142660 [details] [diff] [review]
Avoid early return

+  if (NS_SUCCEEDED(rv)) {
+    PLEvent *ev;
+    while (!listener->mLookupFinished && NS_SUCCEEDED(rv)) {
+      rv = eventQ->WaitForEvent(&ev);
+      NS_ASSERTION(NS_SUCCEEDED(rv), "WaitForEvent failed");
+      if (NS_SUCCEEDED(rv)) {
+	 rv = eventQ->HandleEvent(ev);
+	 NS_ASSERTION(NS_SUCCEEDED(rv), "HandleEvent failed");
+      }
+    }
+    aResult.Assign(listener->mOfficialHostName);
+  }

This checks NS_SUCCEEDED(rv) more than it needs to, either loose the outermost
if check and leave the rest as is, or don't check in the while() condition and
break out of the loop on failure. I.e.

+  PLEvent *ev;
+  while (!listener->mLookupFinished && NS_SUCCEEDED(rv)) {
+    rv = eventQ->WaitForEvent(&ev);
+    NS_ASSERTION(NS_SUCCEEDED(rv), "WaitForEvent failed");
+    if (NS_SUCCEEDED(rv)) {
+      rv = eventQ->HandleEvent(ev);
+      NS_ASSERTION(NS_SUCCEEDED(rv), "HandleEvent failed");
+    }
+  }
+  aResult.Assign(listener->mOfficialHostName);

or:

+  if (NS_SUCCEEDED(rv)) {
+    PLEvent *ev;
+    while (!listener->mLookupFinished) {
+      rv = eventQ->WaitForEvent(&ev);
+      if (NS_FAILED(rv)) {
+	 NS_ERROR("WaitForEvent failed");
+	 break;
+      }
+
+      rv = eventQ->HandleEvent(ev);
+      if (NS_FAILED(rv)) {
+	 NS_ERROR("HandleEvent failed");
+	 break;
+      }
+    }
+
+    aResult.Assign(listener->mOfficialHostName);
+  }

Other than that this looks really good. Well done!

sr=jst

Comment 10

[Harshal Pradhan]

Checked in patch with first variation suggested in comment 9 above. 

-> Fixed.