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)

Assignee

Description

7 years ago
I'm not aware of a reason any of this stuff should indirect through nspr
Assignee

Comment 1

7 years ago
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)
Assignee

Comment 2

7 years ago
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+
Assignee

Comment 4

7 years ago
Attachment #671273 - Flags: review?(mounir)
Assignee

Comment 5

7 years ago
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)
Assignee

Comment 6

7 years ago
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 12

7 years ago
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+
Assignee

Updated

7 years ago
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
Assignee

Comment 24

7 years ago
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
Assignee

Comment 25

7 years ago
(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 :/
Assignee

Comment 27

7 years ago
backed the last one out for asserts remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1782f72408b
Depends on: 818657

Comment 30

7 years ago
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
Assignee

Comment 31

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

Updated

7 years ago
Depends on: 819630
Assignee

Comment 36

7 years ago
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

7 years ago
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.

Updated

7 years ago
No longer depends on: 819630
Assignee

Updated

7 years ago
Depends on: 819927
Assignee

Comment 43

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.