818657 - Fix build failures due to prmem include removals

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Frank Wein [:mcsmurf]]

Bug 801466 removed a few prmem includes from files in mozilla-central. Some files in mailnews/ relied on this and so this now broke.
Example build failures:
e:\builds\moz2_slave\tb-c-cen-w32-dbg\build\config\rules.mk:1113:0$ e:/builds/moz2_slave/tb-c-cen-w32-dbg/build/objdir-tb/_virtualenv/Scripts/python.exe -O e:/builds/moz2_slave/tb-c-cen-w32-dbg/build/mozilla/build/cl.py cl -FonsMsgUtils.obj -c -D_HAS_EXCEPTIONS=0 -I../../../mozilla/dist/stl_wrappers  -D_IMPL_NS_MSG_BASE -DZLIB_DLL -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DMOZ_THUNDERBIRD=1 -DOSTYPE=\"WINNT6.1\" -DOSARCH=WINNT  -Ie:/builds/moz2_slave/tb-c-cen-w32-dbg/build/mailnews/base/util -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Ie:/builds/moz2_slave/tb-c-cen-w32-dbg/build/objdir-tb/mozilla/dist/include/nspr -Ie:/builds/moz2_slave/tb-c-cen-w32-dbg/build/objdir-tb/mozilla/dist/include/nss        -TP -nologo -W3 -Gy -Fdgenerated.pdb -wd4345 -wd4351 -wd4800 -we4553 -GR-  -DDEBUG -D_DEBUG -DTRACING -Zi -O1 -Oy- -MDd           -FI ../../../comm-config.h -DMOZILLA_CLIENT e:/builds/moz2_slave/tb-c-cen-w32-dbg/build/mailnews/base/util/nsMsgUtils.cpp
nsMsgDBFolder.cpp

e:/builds/moz2_slave/tb-c-cen-w32-dbg/build/mailnews/base/util/nsMsgDBFolder.cpp(1733) : warning C4244: 'argument' : conversion from 'int64_t' to 'uint32_t', possible loss of data

e:/builds/moz2_slave/tb-c-cen-w32-dbg/build/mailnews/base/util/nsMsgDBFolder.cpp(2762) : error C3861: 'PR_MALLOC': identifier not found

e:/builds/moz2_slave/tb-c-cen-w32-dbg/build/mailnews/base/util/nsMsgDBFolder.cpp(2779) : error C3861: 'PR_Free': identifier not found
[...]

Comment 1

[:aceman]

Attachment: patch

Comment 2

[Mark Banner (:standard8)]

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

Review of attachment 688955 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not quite sure why we need to dynamically allocate lineBuffer, but for now, we'll keep it as it is.

Just some changes need applying in most of the cases.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +5429,5 @@
>     4. multipart/mixed - scan past boundary, treat next part as body.
>     */
>  
> +  nsAutoPtr<nsLineBuffer<char> > lineBuffer;
> +  lineBuffer = new nsLineBuffer<char>;

nit: We can make this all on one line.

We should also be checking for 

if (!lineBuffer)
  return NS_ERROR_OUT_OF_MEMORY;

Comment 3

[:aceman]

Attachment: patch v2

OK, done.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 688969 [details] [diff] [review]
patch v2

Review of attachment 688969 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

Comment 5

[Mark Banner (:standard8)]

Checked in: https://hg.mozilla.org/comm-central/rev/f6aa8845e67b

Comment 6

[[e-mail]]

nsLineBuffer<char> is 4108/4120 bytes, so possibly unsuitable for stack allocation?

Comment 7

[Trevor Saunders (:tbsaunde)]

> We should also be checking for 
> 
> if (!lineBuffer)
>   return NS_ERROR_OUT_OF_MEMORY;

why? isn't new infalible in com-central?

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 688969 [details] [diff] [review]
patch v2

>@@ -5582,17 +5584,17 @@ NS_IMETHODIMP nsMsgDBFolder::GetMsgTextF
>           msgText.Append(NS_LITERAL_CSTRING("\r\n"));
>       }
> 
>       bytesRead += curLine.Length();
>       if (bytesRead > bytesToRead)
>         break;
>     }
>   }
>-  PR_Free(lineBuffer);
>+  lineBuffer = nullptr;

is there some reason the memory needs to be freed then as opposed to when the variable goes out of scope?

(same comment applies a bunch of places below)

Comment 9

[Mark Banner (:standard8)]

(In reply to [e-mail] from comment #6)
> nsLineBuffer<char> is 4108/4120 bytes, so possibly unsuitable for stack
> allocation?

That would probably be it.

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > We should also be checking for 
> > 
> > if (!lineBuffer)
> >   return NS_ERROR_OUT_OF_MEMORY;
> 
> why? isn't new infalible in com-central?

Well, for one, last I knew, new isn't infallible - I know it was being worked on, but I cannot find any announcements on mozilla.dev.planning about it, nor have I seen a change in status mentioned anywhere else (and I was expecting this to have been shouted about quite a bit).

I know strings are infallible, but not allocators.