801466 - remove a bunch of uses of prmem stuff

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Trevor Saunders (:tbsaunde)]

I'm not aware of a reason any of this stuff should indirect through nspr

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch 1 layout xpcom and libpref

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.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: patch 2 gfx/

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 671269 [details] [diff] [review]
patch 1 layout xpcom and libpref

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

Nice.

Comment 4

[Trevor Saunders (:tbsaunde)]

Attachment: patch 3 content/ dom/ docshell/

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: patch 4 network/ intl/

note that the only real chhange in intl is basically working with the new way nsReadLine from netowrk works.

Comment 6

[Trevor Saunders (:tbsaunde)]

Attachment: patch 5 rdf/ uriloader/

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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()?

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 671272 [details] [diff] [review]
patch 2 gfx/

>+using namespace mozilla;
> using mozilla::CheckedInt;
Is "using mozilla::CheckedInt" still needed?

Comment 9

[(no longer active)]

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.

Comment 10

[Mounir Lamouri (:mounir)]

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];

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 12

[Jason Duell]

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.

Comment 13

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a253800d2e57

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a253800d2e57

Comment 15

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/26ca509c6456

Comment 16

[Mats Palmgren (inactive)]

I suspect this broke my build: bug 817459

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/26ca509c6456

Comment 18

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d13a6a88735b

Comment 19

[Masatoshi Kimura [:emk]]

>    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?

Comment 20

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3d411a8554e0

Comment 21

[Ed Morley [:emorley]]

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

Comment 22

[Ed Morley [:emorley]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d13a6a88735b

Comment 24

[Trevor Saunders (:tbsaunde)]

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

Comment 25

[Trevor Saunders (:tbsaunde)]

(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 :/

Comment 26

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb432c47170

Comment 27

[Trevor Saunders (:tbsaunde)]

backed the last one out for asserts remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1782f72408b

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c0715c079f1e

Comment 29

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/61b7d462bc9e

Comment 30

[:aceman]

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?

Comment 31

[Trevor Saunders (:tbsaunde)]

(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.

Comment 32

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c0679596eb

Comment 33

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c95cee5cd9e

Comment 34

[(no longer active)]

(In reply to Trevor Saunders (:tbsaunde) from comment #33)
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c95cee5cd9e

Backed this out because of the mochitest-1 test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/2971c45877a7

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

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/61b7d462bc9e
https://hg.mozilla.org/mozilla-central/rev/e2c0679596eb

Comment 36

[Trevor Saunders (:tbsaunde)]

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.

Comment 37

[:aceman]

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 :)

Comment 38

[Scoobidiver (away)]

(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.

Comment 39

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2f1dfa1f67

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c2f1dfa1f67

Comment 41

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb270f328b5a

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb270f328b5a

Comment 43

[Trevor Saunders (:tbsaunde)]

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.