Note: There are a few cases of duplicates in user autocompletion which are being worked on.

help @src only works with absolute URLs

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: Allan Beaufour, Assigned: Merle Sterling)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
 
(Reporter)

Comment 1

12 years ago
Created attachment 213755 [details]
Testcase
(Reporter)

Updated

12 years ago
Blocks: 326556
(Reporter)

Updated

12 years ago
No longer blocks: 326556
(Reporter)

Updated

12 years ago
Blocks: 331209

Updated

12 years ago
Blocks: 322255

Comment 2

11 years ago
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

11 years ago
Oh and it fails 8.3.4.b for xf:help element

Comment 4

11 years ago
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

11 years ago
I haven't seen any activity on this bug from Olli and Merle has found out some good stuff debugging it.  Assigning to him.
Assignee: Olli.Pettay → msterlin

Updated

11 years ago
Blocks: 331246
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 years ago
Created attachment 221494 [details]
Testcase - Local relative file
(Assignee)

Comment 7

11 years ago
Created attachment 221495 [details] [diff] [review]
Convert relative urls to absolute

Implement OnStartRequest, OnDataAvailable, and on OnStopRequest.  Convert relative urls to absolute in HandleModalAndModlessMessage.
Attachment #221495 - Flags: review?(aaronr)

Comment 8

11 years ago
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.
Attachment #221495 - Flags: review?(aaronr) → review-
(Assignee)

Comment 9

11 years ago
Created attachment 221740 [details] [diff] [review]
Changes to OnStartRequest
Attachment #221495 - Attachment is obsolete: true
Attachment #221740 - Flags: review?(aaronr)

Comment 10

11 years ago
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
Attachment #221740 - Flags: review?(aaronr) → review+
(Assignee)

Comment 11

11 years ago
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.
Attachment #221740 - Attachment is obsolete: true
Attachment #221830 - Flags: review?(Olli.Pettay)

Comment 12

11 years ago
*** Bug 332005 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Attachment #221830 - Flags: review?(Olli.Pettay) → review+
(Reporter)

Comment 13

11 years ago
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
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.
Created attachment 222083 [details] [diff] [review]
fixed patch for linux

Comment 16

11 years ago
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

11 years ago
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.1
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.0.5
(Reporter)

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.