Last Comment Bug 818657 - Fix build failures due to prmem include removals
: Fix build failures due to prmem include removals
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- blocker (vote)
: Thunderbird 20.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 801466
  Show dependency treegraph
 
Reported: 2012-12-05 13:02 PST by Frank Wein [:mcsmurf]
Modified: 2012-12-05 23:10 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (13.13 KB, patch)
2012-12-05 14:12 PST, :aceman
standard8: review-
Details | Diff | Review
patch v2 (13.58 KB, patch)
2012-12-05 14:49 PST, :aceman
standard8: review+
Details | Diff | Review

Description Frank Wein [:mcsmurf] 2012-12-05 13:02:24 PST
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 2012-12-05 14:12:55 PST
Created attachment 688955 [details] [diff] [review]
patch
Comment 2 Mark Banner (:standard8) 2012-12-05 14:25:16 PST
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 2012-12-05 14:49:48 PST
Created attachment 688969 [details] [diff] [review]
patch v2

OK, done.
Comment 4 Mark Banner (:standard8) 2012-12-05 14:59:33 PST
Comment on attachment 688969 [details] [diff] [review]
patch v2

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

Looks good, thanks.
Comment 5 Mark Banner (:standard8) 2012-12-05 15:00:50 PST
Checked in: https://hg.mozilla.org/comm-central/rev/f6aa8845e67b
Comment 6 neil@parkwaycc.co.uk 2012-12-05 16:18:23 PST
nsLineBuffer<char> is 4108/4120 bytes, so possibly unsuitable for stack allocation?
Comment 7 Trevor Saunders (:tbsaunde) 2012-12-05 20:15:59 PST
> 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) 2012-12-05 20:28:23 PST
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) 2012-12-05 23:10:59 PST
(In reply to neil@parkwaycc.co.uk 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.

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