remove a bunch of uses of prmem stuff

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open])

Attachments

(5 attachments)

I'm not aware of a reason any of this stuff should indirect through nspr
the nsBidiUtils stuff scares me a little because of the way macros call the function with pointers to members, but I believe its correct, and if it isn't PR_Malloc and friends probably end up calling moz_malloc etc anyway so I believe it should still work out.
Attachment #671269 - Flags: review?(roc)
Posted patch patch 2 gfx/Splinter Review
Attachment #671272 - Flags: review?(jmuizelaar)
Comment on attachment 671269 [details] [diff] [review]
patch 1 layout xpcom and libpref

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

Nice.
Attachment #671269 - Flags: review?(roc) → review+
Attachment #671273 - Flags: review?(mounir)
note that the only real chhange in intl is basically working with the new way nsReadLine from netowrk works.
Attachment #671278 - Flags: review?(jduell.mcbugs)
Attachment #671281 - Flags: review?(ehsan)
Comment on attachment 671272 [details] [diff] [review]
patch 2 gfx/

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

::: gfx/thebes/gfxASurface.cpp
@@ +762,5 @@
>    // ...leave a little extra room so we can call read again and make sure we
>    // got everything. 16 bytes for better padding (maybe)
>    bufSize += 16;
>    uint32_t imgSize = 0;
> +  char* imgData = (char*)moz_malloc(bufSize);

why do we need to use moz_malloc() instead of regular malloc()?
Attachment #671272 - Flags: review?(jmuizelaar) → review+
Comment on attachment 671272 [details] [diff] [review]
patch 2 gfx/

>+using namespace mozilla;
> using mozilla::CheckedInt;
Is "using mozilla::CheckedInt" still needed?
Comment on attachment 671281 [details] [diff] [review]
patch 5 rdf/ uriloader/

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

::: rdf/datasource/src/nsFileSystemDataSource.cpp
@@ +878,3 @@
>  
> +        if (NS_FAILED(rv)) return rv;
> +        volumes->AppendElement(vol);

Nit: please fix the indentation.
Attachment #671281 - Flags: review?(ehsan) → review+
Comment on attachment 671273 [details] [diff] [review]
patch 3 content/ dom/ docshell/

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

r=me for everything except the changes in:
nsDOMFileReader::GetAsDataURL (nsDOMFileReader.cpp)

I would like someone who actually knows this code to check if that's fine.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +103,5 @@
>    nsRefPtr<layers::Image> image = mImageContainer->CreateImage(&format, 1);
>  
>    int len = ((DEFAULT_WIDTH * DEFAULT_HEIGHT) * 3 / 2);
>    mImage = static_cast<layers::PlanarYCbCrImage*>(image.get());
> +  nsAutoArrayPtr<uint8_t> frame(new uint8_t[len]);

nit:
I think this is more readable:
nsAutoArrayPtr<uint8_t> frame = new uint8_t[len];

::: dom/file/ArchiveZipEvent.cpp
@@ +151,5 @@
>        return RunShare(NS_ERROR_FILE_CORRUPTED);
>      }
>  
>      // Read the name:
> +    nsAutoArrayPtr<char> filename(new char[filenameLen + 1]);

nit:
I think this is more readable:
nsAutoArrayPtr<char> filename = new char[filenameLen + 1];
Attachment #671273 - Flags: review?(mounir)
Attachment #671273 - Flags: review?(khuey)
Attachment #671273 - Flags: review+
Comment on attachment 671273 [details] [diff] [review]
patch 3 content/ dom/ docshell/

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

If it passes tests, r=me
Attachment #671273 - Flags: review?(khuey) → review+
Comment on attachment 671278 [details] [diff] [review]
patch 4 network/ intl/

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

+r with one fix.  No need for re-review.

::: netwerk/protocol/data/nsDataChannel.cpp
@@ +72,5 @@
>          resultLen = ((resultLen * 3) / 4);
>  
> +        nsAutoCString decodedData;
> +        Base64Decode(dataBuffer, decodedData);
> +        NS_ENSURE_SUCCESS(rv, rv);

You're missing "rv = " before Base64Decode.
Attachment #671278 - Flags: review?(jduell.mcbugs) → review+
Whiteboard: [leave open]
I suspect this broke my build: bug 817459
Depends on: 817459
>    4.34 +  nsAutoArrayPtr<char*> valueArray(new char*[gHashTable.entryCount]);
>    4.35 +  memset(valueArray, 0, gHashTable.entryCount * sizeof(char*));
new char*[gHashTable.entryCount]() didn't work?
Really sorry, had to back out for build failures (OS X so far, others pending):
{
nsAutoPtr.h:73:11: error: implicit instantiation of undefined template 'nsLineBuffer<unsigned short>'
}

Example log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17606792&tree=Mozilla-Inbound

Backout changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b104b77e8b25
Also Linux:
{
nsAutoPtr.h:73:11: error: invalid use of incomplete type 'struct nsLineBuffer<char>'
../../../netwerk/build/../base/src/nsFileStreams.h:23:32: error: declaration of 'struct nsLineBuffer<char>'
../../dist/include/nsAutoPtr.h:73:11: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined.
}

Example log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17607043&tree=Mozilla-Inbound
I'm not sure why gcc 4.5 errors, but 4.7 doesn't, but anyway pushed again as 
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0715c079f1e
(In reply to Masatoshi Kimura [:emk] from comment #19)
> >    4.34 +  nsAutoArrayPtr<char*> valueArray(new char*[gHashTable.entryCount]);
> >    4.35 +  memset(valueArray, 0, gHashTable.entryCount * sizeof(char*));
> new char*[gHashTable.entryCount]() didn't work?

uhm I guess that is enough, and I'm a bit of a clown :/
backed the last one out for asserts remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1782f72408b
Depends on: 818657
You removed NS_InitLineBuffer but http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/public/nsReadLine.h#19 still mentions it. Can you please fix the comment? We had to remove its usage throughout comm-central in bug 818657.

What is the plan in this bug? The description does not say much. Are there any other changes in the queue here?
Status: NEW → ASSIGNED
(In reply to :aceman from comment #30)
> You removed NS_InitLineBuffer but
> http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/public/
> nsReadLine.h#19 still mentions it. Can you please fix the comment? We had to

thanks for catching that.

> remove its usage throughout comm-central in bug 818657.

sorry the current situation sucks for you guys :(

> What is the plan in this bug? The description does not say much. Are there
> any other changes in the queue here?

about half of part 3 still needs to land, but the only thing that might effect you there is that dom blobs are expected to have there buffer allocated with moz_ malloc / realloc / calloc / free instead of PR_xxx, but if you use PR_xxx with them its probably still fine since it'll end up getting redirected to moz_xxx I believe.
Depends on: 819630
I backed out most of part 4 in https://hg.mozilla.org/integration/mozilla-inbound/rev/02a65951d77d  except the parts that would break com central hoping that would be enough to fix the top crashes.
I think even if you revert everything it will not break comm-central now. We just don't use the NS_InitLineBuffer so it does not matter if you put it back.
And we properly included prmem.h where needed so changing if it is included in your Core files does not matter anymore. But thanks for having us in mind :)
(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> I backed out most of part 4 in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/02a65951d77d  except
> the parts that would break com central hoping that would be enough to fix
> the top crashes.
Suspected regressions stopped in 20.0a1/20121208 without any backout in m-c. You can land it again in m-i.
No longer depends on: 819630
Depends on: 819927
the only thing that hasn't landed is the nsJSEnviroment.cpp stuff, which I suspect is old dom bindings only, so I don't really want to spend time investigating it.  There's plenty more prmem stuff around the tree, but I'll called this particular bug fixed.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.