1317726 - sdp_file_parser still depends upon xpcom glue

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Dan Minor [:dminor]]

I chose the wrong moz.build template when making this a standalone binary and it *still* depends upon xpcom glue.

It looks like the only symbol it is using is NS_DebugBreak.

Comment 1

[Nils Ohlmeier [:drno]]

@Dan: if it helps this does not even need to be gtest or whatever. I only made this as a clone of the sdp_unitests because it was the easiest path. Feel free to remove as much of the dependencies. The only functionality it needs is to pass the content of a file to the SDP parser.

Comment 2

[Nils Ohlmeier [:drno]]

Maybe you can also try to fix bug 1317764 at the same time :-)

Comment 3

[Dan Minor [:dminor]]

Attachment: Bug 1317726 - sdp_file_parser should not depend on xpcom glue;

Review commit: https://reviewboard.mozilla.org/r/93450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93450/

Comment 4

[Dan Minor [:dminor]]

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&filter-tier&filter-tier=2&filter-tier=1&selectedJob=31224369

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8811301 [details]
Bug 1317726 - sdp_file_parser should not depend on xpcom glue;

https://reviewboard.mozilla.org/r/93450/#review93548

::: media/webrtc/signaling/src/sdp/sipcc/cpr_string.c:13
(Diff revision 1)
> -#include "mozilla/mozalloc.h"
> -
> -#define cpr_malloc(a) moz_xmalloc(a)
> +#define cpr_malloc(a) malloc(a)
> +#define cpr_calloc(a, b) calloc(a, b)
> +#define cpr_realloc(a, b) realloc(a, b)
> -#define cpr_calloc(a, b) moz_xcalloc(a, b)
> -#define cpr_realloc(a, b) moz_xrealloc(a, b)

I'm not sure what the effect of this change is on Firefox itself.
Would it be possible to only re-define the moz_x*() function to the standard libc function inside the sdp_file_parser itself instead, so that Firefox would still keep using the moz_x* versions?

Comment 6

[Dan Minor [:dminor]]

(In reply to Nils Ohlmeier [:drno] from comment #5)
> Comment on attachment 8811301 [details]
> Bug 1317726 - sdp_file_parser should not depend on xpcom glue;
> 
> https://reviewboard.mozilla.org/r/93450/#review93548
> 
> ::: media/webrtc/signaling/src/sdp/sipcc/cpr_string.c:13
> (Diff revision 1)
> > -#include "mozilla/mozalloc.h"
> > -
> > -#define cpr_malloc(a) moz_xmalloc(a)
> > +#define cpr_malloc(a) malloc(a)
> > +#define cpr_calloc(a, b) calloc(a, b)
> > +#define cpr_realloc(a, b) realloc(a, b)
> > -#define cpr_calloc(a, b) moz_xcalloc(a, b)
> > -#define cpr_realloc(a, b) moz_xrealloc(a, b)
> 
> I'm not sure what the effect of this change is on Firefox itself.
> Would it be possible to only re-define the moz_x*() function to the standard
> libc function inside the sdp_file_parser itself instead, so that Firefox
> would still keep using the moz_x* versions?

Will do!

(My understanding is that these would cause Firefox to use the fallible allocator, which would mean that the results of each call need to be checked to see if they are null before being used. I double checked, and this isn't done consistently in this file and so this change isn't safe. I had initially thought that this would still use the moz_x* calls behind the scenes.)

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8811301 [details]
Bug 1317726 - sdp_file_parser should not depend on xpcom glue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93450/diff/1-2/

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8811301 [details]
Bug 1317726 - sdp_file_parser should not depend on xpcom glue;

https://reviewboard.mozilla.org/r/93450/#review94040

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a36e60d1c4c
sdp_file_parser should not depend on xpcom glue; r=drno

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a36e60d1c4c