119051 - netwerk should provide an extractAttributeValue to read query string params

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Cathleen]

extractAttributeValue is defined in both modoules/libprOn and extensions/inspector

This is breaking static build on windows.  

A temp fix is in static build bug 118044.  
Will need to clean this out.

Comment 1

[timeless]

Here's my proposal:
move the extractAttributeValue function to nsIURLParser where I feel it belongs.

I've rewritten it, and in the process I found the current code and documentation to be below my (nitpicky) standards.
1. "?this=true&his=false","his=" appears to my cursory reading to return "true".
2. the documentation doesn't explain that you need the "=" sign ...

My implementation involves an extra string copy which i'd like feedback on because i know this group is concerned about performance i'd like some comments about it.

I'll describe the impl here because unfortunately i can't upload the file until this evening...

same param structure as current except the param is "his" not "his="

* alloc a new char* that is 3 characters longer than param ("his\0" is 3 chars long, so the allocation will be 6)
* copy the param into the new char starting at [1]. stick in "=\0" as the last two chars
* fill the first char ([0]) with '?'
* run the search as currently used
* if that fails, fill the first char ([0]) with '&', and run the search again
* the alg proceeds about the same as currently except that the checking in the ifs is different (i don't think the current checking is correct)
<q class="example" title="what i think is wrong" src="inspector's_copy">
 95             startOfAttribute += attributeNameSize; // skip over the attributeName
 96             if (startOfAttribute) // is there something after the attribute name
 97             {
 98                 char * endofAttribute = startOfAttribute ? PL_strchr(startOfAttribute, '&') : nsnull;
 99                 if (startOfAttribute && endofAttribute) // is there text after attribute value
</q>
1. we want *startOfAttribute in 95.
2. *startOfAttribute won't be false in 99 and startOfAttribute alone would be useless ...
anyways i've rewritten the code (i got tired so i didn't upload it somewhere) so i don't want to reinvent it this morning.

the main question i have is does anyone have concerns about my extra copy of the param?

Comment 2

[Darin Fisher]

one potential problem with putting this method on nsIURLParser is the fact that
there are multiple nsIURLParser implementations.  that's not a real problem
because currently there is a base class for all the implementations.  it just
might not be the best interface for clients... they would have to get an URL
parser but which one... the std url parser? the auth url parser? the noauth url
parser?  actually, it doesn't matter which one... but they'll have to know that
first.  another choice is to put this method on nsIIOService... there already
are other URL parser methods there.  i almost like this better.  of course
bloating nsIIOService sucks... but it already is bloated.  in the future, i'm
hoping to move the URL functions into some other global service.  nsIURLParser
is really not quite right because of the fact that there are multiple
implementations... and can be per protocol implementations.

Comment 3

[timeless]

Attachment: nsIIOService getQueryAttributeValue

Comment 4

[timeless]

Attachment: inspector doesn't even use the function...

Comment 5

[timeless]

Attachment: convert libpr0n to use nsIIOService

Comment 6

[Darin Fisher]

timeless: your inspector patch is wrong... it has <<<<<<<<'s in it!

Comment 7

[Darin Fisher]

also, i believe mailnews calls extractAttributeValue ... you'll need to patch it
as well.

Comment 8

[Darin Fisher]

Comment on attachment 64772 [details] [diff] [review]
nsIIOService getQueryAttributeValue

>Index: src/nsIOService.cpp

>+nsresult
>+nsIOService::GetQueryAttributeValue(
>+    const char * searchString,
>+    const char * attributeName,
>+    char ** result)
>+{
>+    char * attributeValue = nsnull;
>+    if (searchString && attributeName) { /* search the string for attributeName */
>+        PRUint32 attributeLength = PL_strlen(attributeName);
>+        char * param = new char[attributeLength+3];

this is a very costly way of implementing this function.  it would be much
better
to search for '?' or '&' and then check the following bits for |attributeName|
and
a following '=' instead of constructing a separate search string.

eg. if you had this utility function:

static inline const char *
LocateStartOfAttribute(
    const char *searchString,
    char prefix,
    const char *attr,
    PRUint32 attrLen)
{
    const char *p = PL_strchr(searchString, prefix);
    if (p && !PL_strncasecmp(p+1, attr, attrLen) && p[attrLen+2] == '=')
	return p + 1;
    return nsnull;
}

you could implement GetQueryAttributeValue as such:

nsresult
nsIOService::GetQueryAttributeValue(
    const char * searchString,
    const char * attrName,
    char ** result)
{
    NS_ENSURE_ARG_POINTER(searchString);
    NS_ENSURE_ARG_POINTER(attrName);

    PRUint32 attrLen = nsCRT::strlen(attrName);

    const char * attrStart;

    attrStart = LocateStartOfAttribute(searchString, '?', attrName, attrLen);
    if (!attrStart) {
	attrStart = LocateStartOfAttribute(searchString, '&', attrName,
attrLen);
	if (!attrStart)
	    return NS_ERROR_FAILURE;
    }

    attrStart += attrLen + 1; // skip over the attrName
    // is there something after the attribute name?
    if (*attrStart) {
	const char *attrEnd = PL_strchr(attrStart, '&');
	if (result) {
	    *result = attrEnd
		? // is there text after attribute value
		  PL_strndup(attrStart, attrEnd - attrStart)
		: // there is nothing left so eat up rest of line
		  PL_strdup(attrStart);
	    if (!*result)
		return NS_ERROR_OUT_OF_MEMORY;
	}
	return NS_OK;
    }

    return NS_ERROR_FAILURE;
}

Comment 9

[Darin Fisher]

Comment on attachment 64773 [details] [diff] [review]
inspector doesn't even use the function...


>+<<<<<<< inBitmapURI.cpp
>+=======

this is not a valid patch

Comment 10

[Darin Fisher]

Comment on attachment 64774 [details] [diff] [review]
convert libpr0n to use nsIIOService

>Index: modules/libpr0n/decoders/icon/nsIconURI.cpp

>-  nsCAutoString mozIconPath(aSpec);
>-  PRInt32 pos = mozIconPath.FindChar(NS_MOZ_ICON_DELIMITER);
>+  nsCAutoString mozIconPath(&aSpec[endPos+1]);
>+  rv = ioService->ExtractURLPart(aSpec, nsIIOService::url_Query, &startPos, &endPos, nsnull);
> 
>-  if (pos == -1) // no size or content type specified
>-  {
>-    mozIconPath.Right(mDummyFilePath, mozIconPath.Length() - endPos);
>-  }
>-  else
>+  if (startPos > -1) // no size or content type specified
>   {
>-    mozIconPath.Mid(mDummyFilePath, endPos, pos - endPos);
>+    mozIconPath.Left(mDummyFilePath, startPos);

why make these changes?

Comment 11

[Brendan Eich [:brendan]]

Stop using PL_str*, ok?

/be

Comment 12

[Darin Fisher]

but, is |strchr| really available XP?  i thought it was called |index| on BSD
systems.  and what about strdup and strndup?  we could use the nsCRT:: versions
of those.  seems like strchr needs to be included in nsCRT::.

Comment 13

[Brendan Eich [:brendan]]

The JS engine uses strchr in good health, and ports to FreeBSD and OpenBSD. 
We're finally out of the 90's.  Disco will never die, but the C standard can now
be used, at least for str* and mem* -- and compilers such as gcc will optimize
calls to such methods with constant count params.

/be

Comment 14

[timeless]

Attachment: darin's rewrite, less PL_

Comment 15

[timeless]

Attachment: -w for the mail changes

Comment 16

[Darin Fisher]

Comment on attachment 65027 [details] [diff] [review]
darin's rewrite, less PL_

>Index: netwerk/base/src/nsIOService.cpp

>+    if (p && !strncasecmp(p+1, attr, attrLen) && p[attrLen+2] == '=')

sorry, but on windows there is no strncasecmp, it's called strnicmp... how
about just using nsCRT::strncasecmp for this one?

Comment 17

[timeless]

Attachment: it builds on w32 it must be right

Comment 18

[Darin Fisher]

Comment on attachment 65111 [details] [diff] [review]
it builds on w32 it must be right

sr=darin

Comment 19

[David Bradley]

Comment on attachment 65111 [details] [diff] [review]
it builds on w32 it must be right

r=dbradley

Probably should be checking the return value of the call to
GetQueryAttributeValue for messageKey instead of the one retrieving m_MessageID

Consider changing the strdup in the terinary statement to nsCRT::strdup to be
symetric with the nsCRT::strndup

Comment 20

[timeless]

Attachment: trying to use PL_ and nsCRT:: less but hopefully in matched sets

Comment 21

[timeless]

committed

Comment 22

[timeless]

:)

Comment 23

[(not reading, please use [e-mail] instead)]

this caused a smoketest blocker bug #120324

was the mail code tested?

Comment 24

[(not reading, please use [e-mail] instead)]

the lack of testing and lack of module owner review on this patch is very 
frustrating.

this is sure to get backed out if we can't come up with a solution in the 
blocker bug.

Comment 25

[Scott MacGregor]

What does it take to get some module owner approval or review before checkin in
changes to nsMailboxUrl before checking in? The changes to that file clearly
were never even tested as this routine is now completely broken. Please see
todays smoketest blocker: 120324. Mailbox urls are no longer working so you
can't read local mail. This needs backed out so we can open the tree.

Comment 26

[Scott MacGregor]

sorry I didn't realize Seth already added comments to this bug which I duplicated. 

Comment 27

[Brendan Eich [:brendan]]

We rely on the patch bearer and the super-reviewers (and any helpful onlookers)
to  ensure that module owner or peer (if owner-less, owner is unavailable, or
peer is good enough) gets to r=.  Exceptions are pan-tree changes to fix build 
or #include or some such (string-fu) systematic, module-agnostic problem, and
are noted as exceptions in the bug -- these are rare.

Not only did the system break down here, the reviewers didn't find the bug, and
timeless didn't test enough.  Shame all around.

My shame is having such an honor system that fails too often (apparently, too
often at mailnews' expense).  Suggestions?

One thing I think we could push (I'll add it to reviewers.html) is this: break
patches up by module, so you get separate r= and sr= recordings.

/be

Comment 28

[(not reading, please use [e-mail] instead)]

re-opening, this is being backed out.

Comment 29

[timeless]

Attachment: patch that addresses the quirk of the consumers

ok, new patch, i've been running with the mail changes for a long time. the
libpr0n changes aren't really interesting.  sspitzer: please try this out, i'm
having caillon and rkaa test on linux too.

I'd really like to get this in because it's an API that consumers need and the
comments and code for the current abusers are wrong and broken respectively.

Comment 30

[timeless]

wrt PR_FREEIF, i think that's probably the wrong call, it should probably be 
nsCRT::free

so

<+        m_mailboxAction = NS_SUCCEEDED(rv) 
<+          ? nsIMailboxUrl::ActionFetchPart
<+          : nsIMailboxUrl::ActionFetchMessage;
<+        PR_FREEIF(msgPart);
>+        if (NS_SUCCEEDED(rv)) {
>+          m_mailboxAction = nsIMailboxUrl::ActionFetchPart;
>+          nsCRT::free(msgPart);
>+        } else m_mailboxAction = nsIMailboxUrl::ActionFetchMessage;

 +        if (NS_SUCCEEDED(rv)) {
 +            m_messageKey = atol(messageKey); // convert to a long...
-+            PR_FREEIF(messageKey);
++            nsCRT::free(messageKey);
 +        }

Comment 31

[(not reading, please use [e-mail] instead)]

> sspitzer: please try this out

You just need the last patch tested, right?

Comment 32

[Alex Vincent [:WeirdAl]]

Patch is nearly a year old:  bit rot?  I don't want to add review, sr request
flags to that (particularly since I don't know C++)

Comment 33

[Alex Vincent [:WeirdAl]]

timeless: patch is now over two and a half years old.  Is it still current?

Comment 34

[Shawn Wilsher :sdwilsh]

what's the status of this?

Comment 35

[Phil Ringnalda (:philor)]

The status is that it's (at least) three bugs:

* a netwerk bug to add extractAttributeValue (somewhere unfrozen, since it missed nsIIOService's freeze in October 2002)

* a libpr0n bug to switch nsIconURI to use it

* a mail backend bug to switch nsMailboxUrl to use it

(* possibly another one or more to convert other consumers who happened to name theirs something else)

none with anything to do with DOMi, which shed its extractAttributeValue between 1.4 and 1.7. Not sure anyone wants to fight with any of those anymore, though if they do and if they split the patch up and ask for review with a flag, I'll make sure mailnews doesn't drop the ball the next time.