Last Comment Bug 329106 - help @src only works with absolute URLs
: help @src only works with absolute URLs
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Merle Sterling
: Stephen Pride
:
Mentors:
http://www.w3.org/TR/xforms/
: 332005 (view as bug list)
Depends on:
Blocks: 322255 331209 331246
  Show dependency treegraph
 
Reported: 2006-03-02 08:28 PST by Allan Beaufour
Modified: 2016-07-15 14:46 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Testcase (1015 bytes, application/xhtml+xml)
2006-03-02 08:29 PST, Allan Beaufour
no flags Details
Testcase - Local relative file (974 bytes, text/xml)
2006-05-09 14:10 PDT, Merle Sterling
no flags Details
Convert relative urls to absolute (8.29 KB, patch)
2006-05-09 14:14 PDT, Merle Sterling
aaronr: review-
Details | Diff | Splinter Review
Changes to OnStartRequest (8.10 KB, patch)
2006-05-11 13:36 PDT, Merle Sterling
aaronr: review+
Details | Diff | Splinter Review
Simplify OnStartRequest a bit (7.42 KB, patch)
2006-05-12 11:27 PDT, Merle Sterling
bugs: review+
Details | Diff | Splinter Review
fixed patch for linux (7.21 KB, patch)
2006-05-15 13:23 PDT, Doron Rosenberg (IBM)
no flags Details | Diff | Splinter Review

Description Allan Beaufour 2006-03-02 08:28:33 PST
 
Comment 1 Allan Beaufour 2006-03-02 08:29:05 PST
Created attachment 213755 [details]
Testcase
Comment 2 Steve Speicher 2006-04-13 12:00:57 PDT
This affects some of the test suite, as it depends on local resources: 8.3.5.b (hint) and 8.3.6.b (alert)
Comment 3 Steve Speicher 2006-04-13 12:04:24 PDT
Oh and it fails 8.3.4.b for xf:help element
Comment 4 Steve Speicher 2006-04-24 10:58:16 PDT
Appears src on instance behaves the same way as well, see test case 4.5.2.a
 http://www.w3.org/MarkUp/Forms/Test/XForms1.0/Edition2/Chapt4/4.5/4.5.2/4.5.2.a.xhtml

Perhaps fixing this bug will fix this testcase as well
Comment 5 aaronr 2006-05-01 13:48:31 PDT
I haven't seen any activity on this bug from Olli and Merle has found out some good stuff debugging it.  Assigning to him.
Comment 6 Merle Sterling 2006-05-09 14:10:24 PDT
Created attachment 221494 [details]
Testcase - Local relative file
Comment 7 Merle Sterling 2006-05-09 14:14:11 PDT
Created attachment 221495 [details] [diff] [review]
Convert relative urls to absolute

Implement OnStartRequest, OnDataAvailable, and on OnStopRequest.  Convert relative urls to absolute in HandleModalAndModlessMessage.
Comment 8 aaronr 2006-05-10 14:30:37 PDT
Comment on attachment 221495 [details] [diff] [review]
Convert relative urls to absolute

>Index: nsXFormsMessageElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsMessageElement.cpp,v
>retrieving revision 1.19
>diff -u -8 -p -r1.19 nsXFormsMessageElement.cpp
>--- nsXFormsMessageElement.cpp	17 Apr 2006 13:06:08 -0000	1.19
>+++ nsXFormsMessageElement.cpp	9 May 2006 21:07:50 -0000
>@@ -637,17 +637,30 @@ nsXFormsMessageElement::HandleModalAndMo
>   }
> 
>   // order of precedence is single-node binding, linking attribute then
>   // inline text
>   nsresult rv;
>   if (!hasBinding && !src.IsEmpty()) {
>     // Creating a normal window for messages with src attribute.
>     options.AppendLiteral(",chrome=no");
>-    rv = ConstructMessageWindowURL(src, PR_TRUE, src);
>+
>+    // Create a new URI so that we properly convert relative urls to absolute.
>+    mElement->GetOwnerDocument(&aDoc);

don't need this call to GetOwnerDocument.  aDoc is already mElement's owner document.

>+    nsCOMPtr<nsIDocument> doc(do_QueryInterface(aDoc));
>+    NS_ENSURE_STATE(doc);
>+    nsCOMPtr<nsIURI> uri;
>+    NS_NewURI(getter_AddRefs(uri), src, doc->GetDocumentCharacterSet().get(),
>+              doc->GetDocumentURI());
>+    NS_ENSURE_STATE(uri);
>+    nsCAutoString uriSpec;
>+    uri->GetSpec(uriSpec);
>+    rv = ConstructMessageWindowURL(NS_ConvertUTF8toUTF16(uriSpec),
>+                                   PR_TRUE,
>+                                   NS_ConvertUTF8toUTF16(uriSpec));

you need to pass in the variable 'src' as the 3rd parameter to ConstructMessageWindowURL or your call to internal->OpenDialog later in the function won't have the correct stuff in its 1st parameter.

>     NS_ENSURE_SUCCESS(rv, rv);
>   } else {
>     // Cloning the content of the xf:message and creating a
>     // dialog for it.
>     options.AppendLiteral(",dialog,chrome,dependent,width=200,height=200");
>     nsCOMPtr<nsIDOMDocument> ddoc;
>     nsCOMPtr<nsIDOMDOMImplementation> domImpl;
>     rv = aDoc->GetImplementation(getter_AddRefs(domImpl));
>@@ -1028,88 +1041,96 @@ nsXFormsMessageElement::OnChannelRedirec
> }
> 
> // nsIStreamListener
> 
> NS_IMETHODIMP
> nsXFormsMessageElement::OnStartRequest(nsIRequest *aRequest,
>                                        nsISupports *aContext)
> {
>-  return NS_OK;
>-}
>+    // Make sure to null out mChannel before we return.  Keep in mind that
>+    // if this is the last message channel to be loaded for the xforms
>+    // document then when AddRemoveExternalResource is called, it may result
>+    // in xforms-ready firing. Should there be a message acting as a handler
>+    // for xforms-ready, it will start the logic to display itself
>+    // (HandleAction()).  So we can't call AddRemoveExternalResource to remove
>+    // this channel from the count until we've set the mStopType to be the
>+    // proper value.  Entering this function, mStopType will be eStopType_None,
>+    // so if we need mStopType to be any other value (like in an error
>+    // condition), please make sure it is set before AddRemoveExternalResource
>+    // is called.

nit: the contents of this whole function needs to be moved two columns left to be consistent with the rest of the file.  And please make sure that the contents of any other code blocks (like some of the 'if' tests below) are only indented two spaces.

>+    NS_ASSERTION(aRequest == mChannel, "unexpected request");
>+    NS_ASSERTION(mChannel, "no channel");
>+
>+    nsresult status;
>+    nsresult rv = mChannel->GetStatus(&status);
>+    // DNS errors and other obvious problems will return failure status
>+    if (NS_FAILED(rv) || NS_FAILED(status)) {
>+        // NS_BINDING_ABORTED means that we have been cancelled by a later
>+        // AttributeSet() call (which will also reset mStopType), so don't
>+        // treat it like an error.
>+        if (status != NS_BINDING_ABORTED) {
>+          nsAutoString src, tagName;
>+          mElement->GetLocalName(tagName);
>+          mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
>+          const PRUnichar *strings[] = { tagName.get(), src.get() };
>+          nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"),
>+                                     strings, 2, mElement, mElement);
>+          mStopType = eStopType_LinkError;
>+          AddRemoveExternalResource(PR_FALSE);
>+          mChannel = nsnull;
>+        }
> 
>-NS_IMETHODIMP
>-nsXFormsMessageElement::OnDataAvailable(nsIRequest *aRequest,
>-                                        nsISupports *aContext,
>-                                        nsIInputStream *aInputStream,
>-                                        PRUint32 aOffset,
>-                                        PRUint32 aCount)
>-{
>-  return NS_OK;
>-}
>+          return NS_BINDING_FAILED;
>+    }

nit: return doesn't line up right.

> 
>-NS_IMETHODIMP
>-nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest,
>-                                      nsISupports *aContext,
>-                                      nsresult aStatusCode)
>-{
>+    // If status is zero, it might still be an error if it's http:
>+    // http has data even when there's an error like a 404.
>+    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
>+    if (!httpChannel)
>+        return NS_BINDING_ABORTED;

don't return from ::OnStartRequest, in any manner, without both setting mChannel to nsnull and calling AddRemoveExternalResource(PR_FALSE).  Maybe use a test similar to what currently exists in ::OnStopRequest?  Then you only have to do it in two places in this function rather than three.
Comment 9 Merle Sterling 2006-05-11 13:36:59 PDT
Created attachment 221740 [details] [diff] [review]
Changes to OnStartRequest
Comment 10 aaronr 2006-05-11 17:30:37 PDT
Comment on attachment 221740 [details] [diff] [review]
Changes to OnStartRequest

>Index: nsXFormsMessageElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsMessageElement.cpp,v
>retrieving revision 1.19
>diff -u -8 -p -r1.19 nsXFormsMessageElement.cpp
>--- nsXFormsMessageElement.cpp	17 Apr 2006 13:06:08 -0000	1.19
>+++ nsXFormsMessageElement.cpp	11 May 2006 20:33:44 -0000

>@@ -1028,88 +1040,105 @@ nsXFormsMessageElement::OnChannelRedirec
> }
> 
> // nsIStreamListener
> 
> NS_IMETHODIMP
> nsXFormsMessageElement::OnStartRequest(nsIRequest *aRequest,
>                                        nsISupports *aContext)
> {
>-  return NS_OK;
>-}
>-
>-NS_IMETHODIMP
>-nsXFormsMessageElement::OnDataAvailable(nsIRequest *aRequest,
>-                                        nsISupports *aContext,
>-                                        nsIInputStream *aInputStream,
>-                                        PRUint32 aOffset,
>-                                        PRUint32 aCount)
>-{
>-  return NS_OK;
>-}
>-
>-NS_IMETHODIMP
>-nsXFormsMessageElement::OnStopRequest(nsIRequest *aRequest,
>-                                      nsISupports *aContext,
>-                                      nsresult aStatusCode)
>-{
>-
>-  // We are done with the load request, whatever the result, so make sure to
>-  // null out mChannel before we return.  Keep in mind that if this is the last
>-  // message channel to be loaded for the xforms document then when
>-  // AddRemoveExternalResource is called, it may result in xforms-ready firing.
>-  // Should there be a message acting as a handler for xforms-ready, it will
>-  // start the logic to display itself (HandleAction()).  So we can't call
>-  // AddRemoveExternalResource to remove this channel from the count until we've
>-  // set the mStopType to be the proper value.  Entering this function,
>-  // mStopType will be eStopType_None, so if we need mStopType to be any other
>-  // value (like in an error condition), please make sure it is set before
>-  // AddRemoveExternalResource is called.
>-
>-  if (NS_FAILED(aStatusCode)) {
>+  // Make sure to null out mChannel before we return.  Keep in mind that
>+  // if this is the last message channel to be loaded for the xforms
>+  // document then when AddRemoveExternalResource is called, it may result
>+  // in xforms-ready firing. Should there be a message acting as a handler
>+  // for xforms-ready, it will start the logic to display itself
>+  // (HandleAction()).  So we can't call AddRemoveExternalResource to remove
>+  // this channel from the count until we've set the mStopType to be the
>+  // proper value.  Entering this function, mStopType will be eStopType_None,
>+  // so if we need mStopType to be any other value (like in an error
>+  // condition), please make sure it is set before AddRemoveExternalResource
>+  // is called.
>+  NS_ASSERTION(aRequest == mChannel, "unexpected request");
>+  NS_ASSERTION(mChannel, "no channel");
>+
>+  nsresult status;
>+  nsresult rv = mChannel->GetStatus(&status);
>+  // DNS errors and other obvious problems will return failure status
>+  if (NS_FAILED(rv) || NS_FAILED(status)) {
>     // NS_BINDING_ABORTED means that we have been cancelled by a later
>-    // AttributeSet() call (which will also reset mStopType).  So don't treat
>-    // like an error.
>-    if (aStatusCode != NS_BINDING_ABORTED) {
>+    // AttributeSet() call (which will also reset mStopType), so don't
>+    // treat it like an error.
>+    if (status != NS_BINDING_ABORTED) {
>       nsAutoString src, tagName;
>       mElement->GetLocalName(tagName);
>       mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
>       const PRUnichar *strings[] = { tagName.get(), src.get() };
>       nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"),
>                                  strings, 2, mElement, mElement);
>       mStopType = eStopType_LinkError;
>-      AddRemoveExternalResource(PR_FALSE);
>-      mChannel = nsnull;
>-    
>-      return NS_OK;
>     }
>+
>+    AddRemoveExternalResource(PR_FALSE);
>+    mChannel = nsnull;
>+
>+    return NS_BINDING_FAILED;
>   }
> 
>-  PRUint32 responseStatus;
>+  // If status is zero, it might still be an error if it's http:
>+  // http has data even when there's an error like a 404.
>   nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
>-  if (NS_SUCCEEDED(aStatusCode) && httpChannel) {
>-    nsresult rv = httpChannel->GetResponseStatus(&responseStatus);
>+  if (!httpChannel) {
>+     AddRemoveExternalResource(PR_TRUE);
>+     mChannel = nsnull;
>+     return NS_BINDING_ABORTED;
>+  }
>+

this should be AddRemoveExternalResource(PR_FALSE).  PR_TRUE is for when you are starting a request, I believe.

>+  // If responseStatus is 2xx, it is valid.
>+  // 3xx (various flavors of redirection) COULD be successful.  Can't really
>+  // follow those to conclusion so we'll assume they were successful.
>+  // If responseStatus is 4xx or 5xx, it is an error.
>+  PRUint32 responseStatus;
>+  rv = httpChannel->GetResponseStatus(&responseStatus);
>+  if (NS_FAILED(rv) || (responseStatus >= 400)) {
>+    nsAutoString src, tagName;
>+    mElement->GetLocalName(tagName);
>+    mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
>+    const PRUnichar *strings[] = { tagName.get(), src.get() };
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"),
>+                               strings, 2, mElement, mElement);
>+    mStopType = eStopType_LinkError;
>     
>-    // If responseStatus is 4xx or 5xx, it is an error.  2xx is success.
>-    // 3xx (various flavors of redirection) COULD be successful.  Can't really
>-    //  follow those to conclusion so we'll assume they were successful.
>-    if (NS_FAILED(rv) || (responseStatus >= 400)) {
>-      nsAutoString src, tagName;
>-      mElement->GetLocalName(tagName);
>-      mElement->GetAttribute(NS_LITERAL_STRING("src"), src);
>-      const PRUnichar *strings[] = { tagName.get(), src.get() };
>-      nsXFormsUtils::ReportError(NS_LITERAL_STRING("externalLink2Error"),
>-                                 strings, 2, mElement, mElement);
>-      mStopType = eStopType_LinkError;
>-    }
>+    AddRemoveExternalResource(PR_FALSE);
>+    mChannel = nsnull;
>+
>+    return NS_BINDING_FAILED;
>   }
> 
>   AddRemoveExternalResource(PR_FALSE);
>   mChannel = nsnull;
> 
>+  return NS_BINDING_ABORTED;
>+}
>+

Sorry that I missed this in the first review...you can return NS_BINDING_ABORTED in every situation in ::OnStartRequest.  That should help simplify your code a little down to only two returns in the function, probably.  You shouldn't need to worry about differentiating between NS_BINDING_FAILED and NS_BINDING_ABORTED.  Once we make our test we just want to stop the channel.  I don't think that it matters how we do it since we aren't checking the status anywhere down the line (not in ::OnDataAvailable or ::OnStopRequest, for example).

With those, r=me
Comment 11 Merle Sterling 2006-05-12 11:27:23 PDT
Created attachment 221830 [details] [diff] [review]
Simplify OnStartRequest a bit

Always return NS_BINDING_ABORTED since we don't care to read the data anyway.  Simplify the logic a bit to reduce the number of places from which we return.
Comment 12 aaronr 2006-05-12 11:36:55 PDT
*** Bug 332005 has been marked as a duplicate of this bug. ***
Comment 13 Allan Beaufour 2006-05-15 07:09:07 PDT
nsXFormsMessageElement.cpp: In member function ‘nsresult nsXFormsMessageElement::HandleModalAndModelessMessage(nsIDOMDocument*, nsAString_internal&)’:
nsXFormsMessageElement.cpp:657: error: no matching function for call to ‘nsXFormsMessageElement::ConstructMessageWindowURL(NS_ConvertUTF8toUTF16, int, nsAutoString&)’
nsXFormsMessageElement.cpp:183: note: candidates are: nsresult nsXFormsMessageElement::ConstructMessageWindowURL(nsAString_internal&, PRBool, nsAString_internal&)
gmake[1]: *** [nsXFormsMessageElement.o] Error 1
Comment 14 Doron Rosenberg (IBM) 2006-05-15 13:07:43 PDT
The issue here is that NS_ConvertUTF8toUTF16 creates a const nsAString&, yet ConstructMessageWindowURL just uses nsAString&.

jag: Temporary objects can only bind to const references
jag: MS VC lets them bind to non-const refs too

But Visual C++ seems to allow non-const refs, which linux doesn't.
Comment 15 Doron Rosenberg (IBM) 2006-05-15 13:23:04 PDT
Created attachment 222083 [details] [diff] [review]
fixed patch for linux
Comment 16 Steve Speicher 2006-05-16 05:46:40 PDT
Built with Doron's latest patch and here are the results running W3C test suite:

Test cases now pass:
  8.3.5.b hint
  8.3.6.b alert
  8.3.4.b help
  10.1.12.a valid external message
  10.1.12.b source precedence for message element

Test cases that still fail:
  8.3.5.b linking precedence for hint element
  4.5.2.a xforms-link-exception
...investigating these a bit more
Comment 17 aaronr 2006-05-16 12:27:25 PDT
checked into trunk for msterlin

Note You need to log in before you can comment on or make changes to this bug.