719776 - [Azure] External dependencies slipped into Azure

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Some external dependencies on Azure have slipped into GFX code. It would be nice to not generate these dependencies so stand-alone Azure can work with Quartz, Skia and Direct2D, also in the interest of being able to run a stand-alone test environment.

  C:\Users\Bas\Dev\mozilla-inbound\gfx\2d\DrawTargetSkia.cpp(52):#include "gfxImageSurface.h"
  C:\Users\Bas\Dev\mozilla-inbound\gfx\2d\DrawTargetSkia.h(47):#include "gfxImageSurface.h"
  C:\Users\Bas\Dev\mozilla-inbound\gfx\2d\ScaledFontBase.cpp(47):#include "gfxFont.h"
  C:\Users\Bas\Dev\mozilla-inbound\gfx\2d\ScaledFontCairo.cpp(42):#include "gfxFont.h"
  C:\Users\Bas\Dev\mozilla-inbound\gfx\2d\ScaledFontWin.h(42):#include "gfxGDIFont.h"

The cairo one is less bad since we don't build with stand-alone cairo anyway.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix most of these

Fixes everything except ScaledFontFreetype. This one probably needs us to handle FT_Face properly, I think George was working on this.

Comment 2

[Matt Woodrow (:mattwoodrow)]

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

Comment 3

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f2f7fd9dce - a wee bit burny, that 'un was.

Comment 4

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/690cba3bb817

Comment 5

[(no longer active)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef36ed1ae27 because of build problems.  Please test on the try server before landing again!

Comment 6

[Matt Woodrow (:mattwoodrow)]

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

Comment 7

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Some additional fixes to make Azure build stand-alone again.

This fixes the last dependencies, Jeff, could you verify this realloc change is ok? Since we don't want to link against mozalloc stand-alone (it's tricky) I'd rather not include it here.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 612363 [details] [diff] [review]
Part 2: Some additional fixes to make Azure build stand-alone again.

Can we just abort instead of returning?

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 612363 [details] [diff] [review]
> Part 2: Some additional fixes to make Azure build stand-alone again.
> 
> Can we just abort instead of returning?

I couldn't find any abort code in mfbt? I could add a local Abort function that just does a *((int*)0x0) = 123; or something of the likes?

Comment 10

[(no longer active)]

(In reply to Bas Schouten (:bas) from comment #9)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > Comment on attachment 612363 [details] [diff] [review]
> > Part 2: Some additional fixes to make Azure build stand-alone again.
> > 
> > Can we just abort instead of returning?
> 
> I couldn't find any abort code in mfbt? I could add a local Abort function
> that just does a *((int*)0x0) = 123; or something of the likes?

Use MOZ_Crash: http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#135

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> (In reply to Bas Schouten (:bas) from comment #9)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > > Comment on attachment 612363 [details] [diff] [review]
> > > Part 2: Some additional fixes to make Azure build stand-alone again.
> > > 
> > > Can we just abort instead of returning?
> > 
> > I couldn't find any abort code in mfbt? I could add a local Abort function
> > that just does a *((int*)0x0) = 123; or something of the likes?
> 
> Use MOZ_Crash:
> http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#135

Looks like that is externally defined so would drag in some dependency. Not sure how bad that dependency is though. As I'm not sure what library defines it.

Comment 12

[(no longer active)]

(In reply to Bas Schouten (:bas) from comment #11)
> (In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > (In reply to Bas Schouten (:bas) from comment #9)
> > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > > > Comment on attachment 612363 [details] [diff] [review]
> > > > Part 2: Some additional fixes to make Azure build stand-alone again.
> > > > 
> > > > Can we just abort instead of returning?
> > > 
> > > I couldn't find any abort code in mfbt? I could add a local Abort function
> > > that just does a *((int*)0x0) = 123; or something of the likes?
> > 
> > Use MOZ_Crash:
> > http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#135
> 
> Looks like that is externally defined so would drag in some dependency. Not
> sure how bad that dependency is though. As I'm not sure what library defines
> it.

I'm not sure where the symbol is defined.  Waldo would know.  And hey may even be fine with you changing that function to be inline.

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3b2230d34d3f

Comment 14

[Jeff Walden [:Waldo]]

(In reply to Bas Schouten (:bas) from comment #11)
> Looks like that is externally defined so would drag in some dependency. Not
> sure how bad that dependency is though. As I'm not sure what library defines
> it.

If you're using MOZ_ASSERT already, or any of the mfbt classes (RefPtr, say), you're already depending on the library that provides MOZ_Crash and MOZ_Assert.  So probably that boat's already sailed, as far as you're concerned.

(I think it might be a good idea to change MOZ_ASSERT to expand to inline code that will report an assertion and halt.  And MOZ_Crash, maybe renamed MOZ_CRASH(msg), should probably do about the same.  The big advantage to doing this is that it wouldn't affect crash stacks, and if you were debugging, the debugger would be parked on the location of the assertion, not on the line of code within MOZ_Crash that caused the int3 or whatever.  If those changes were made, there would be no library dependency here at all.  It hasn't been a priority for me to change this thus far.)

Comment 15

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #14)
> (In reply to Bas Schouten (:bas) from comment #11)
> > Looks like that is externally defined so would drag in some dependency. Not
> > sure how bad that dependency is though. As I'm not sure what library defines
> > it.
> 
> If you're using MOZ_ASSERT already, or any of the mfbt classes (RefPtr,
> say), you're already depending on the library that provides MOZ_Crash and
> MOZ_Assert.  So probably that boat's already sailed, as far as you're
> concerned.
> 
> (I think it might be a good idea to change MOZ_ASSERT to expand to inline
> code that will report an assertion and halt.  And MOZ_Crash, maybe renamed
> MOZ_CRASH(msg), should probably do about the same.  The big advantage to
> doing this is that it wouldn't affect crash stacks, and if you were
> debugging, the debugger would be parked on the location of the assertion,
> not on the line of code within MOZ_Crash that caused the int3 or whatever. 
> If those changes were made, there would be no library dependency here at
> all.  It hasn't been a priority for me to change this thus far.)

RefPtr is not giving any issues, it's fully inlined, I'm unsure why MOZ_ASSERT isn't giving issues, but it's nice that it doesn't!

My stand-alone builds run just fine here :)

Comment 16

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Inline MOZ_Assert and MOZ_Crash

Comment 17

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Fix Azure stand-alone build.

Comment 18

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Inline MOZ_Assert and MOZ_Crash

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 613798 [details] [diff] [review]
Part 2: Inline MOZ_Assert and MOZ_Crash

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

This is definitely the right track, should be much nicer for debugging and for Socorro and all as far as crash stacks and such go.  Mostly I've got formatting nits here, but there are enough of them I probably should take a quick look at a revised version.  Ping me on IRC for fast turnaround if you can.

::: js/public/Utility.h
@@ +366,5 @@
> + */
> +static MOZ_ALWAYS_INLINE size_t
> +JS_FLOOR_LOG2W(size_t n)
> +{
> +    JS_ASSERT((n) != 0);

Get rid of the excess parentheses.

::: js/src/jscntxt.h
@@ +1569,5 @@
> +static MOZ_ALWAYS_INLINE bool
> +JS_CHECK_OPERATION_LIMIT(JSContext *cx)
> +{
> +    JS_ASSERT_REQUEST_DEPTH(cx);
> +    return (!cx->runtime->interrupt || js_InvokeOperationCallback(cx));

Remove the outer, now-unnecessary parentheses.

::: mfbt/Assertions.h
@@ +48,5 @@
>  
> +#include <stdio.h>
> +#include <stdlib.h>
> +#ifndef WIN32
> +#include <signal.h>

mfbt/STYLE doesn't say this right now (I'll update it to say so soon), but nested includes like this should be indented two spaces, like this:

#ifndef WIN32
#  include <signal.h>
#endif

This is consistent with the rest of our nested-preprocessor-instruction style, and it makes clearer that the include is only conditional.

@@ +52,5 @@
> +#include <signal.h>
> +#endif
> +#ifdef ANDROID
> +#include <android/log.h>
> +#endif

And

#ifdef ANDROID
#  include <android/log.h>
#endif

@@ +143,5 @@
> +   * We write 123 here so that the machine code for this function is
> +   * unique. Otherwise the linker, trying to be smart, might use the
> +   * same code for MOZ_Crash and for some other function. That
> +   * messes up the signature in minidumps.
> +   */

This comment seems no longer relevant, now that all this will get inlined at the using site.  Remove it?

@@ +148,3 @@
>  
> +#if defined(WIN32)
> +  /*

This comment block should be indented three spaces, not two, so that the '/' lines up with the 'd' in 'define' below it.  Likewise for the comments on the other #defines nested within this extended #if-block.

@@ +150,5 @@
> +  /*
> +   * We used to call DebugBreak() on Windows, but amazingly, it causes
> +   * the MSVS 2010 debugger not to be able to recover a call stack.
> +   */
> +#define MOZ_CRASH() \

This should be indented like so:

#if defined(WIN32)
   /*
    * ...
    */
#  define MOZ_CRASH() \
     do { \
       *((volatile int *) NULL) = 123; \
       exit(3); \
     } while (0)

@@ +153,5 @@
> +   */
> +#define MOZ_CRASH() \
> +  do { \
> +      *((volatile int *) NULL) = 123; \
> +      exit(3); \

Block indentation in mfbt is two spaces, so this body is over-indented by two.

@@ +156,5 @@
> +      *((volatile int *) NULL) = 123; \
> +      exit(3); \
> +  } while (0)
> +
> +#elif defined(ANDROID)

No blank line above this (and in the similar cases later).  Since this entire block of preprocessor stuff is dedicated to defining a single macro, it's most clearly one coherent whole if it's not broken up with whitespace, but only broken up through indentation.

@@ +164,5 @@
> +   */
> +#define MOZ_CRASH() \
> +  do { \
> +      *((volatile int *) NULL) = 123; \
> +      abort(); \

Block indentation.  (There are a bunch more, I'll stop noting them now.)

@@ +174,5 @@
> +   * trapped.
> +   */
> +#define MOZ_CRASH() \
> +  do { \
> +      *((volatile int *) NULL) = 123;  /* To continue from here in GDB: "return" then "continue". */ \

This comment is no longer accurate, and indeed it probably won't even be seen most of the time.  For now I'm fine with removing it, as we lived without it for some time before it got added.  But we should readd a comment explaining the right way to do it at some point.  Please file a bug for this if you don't know the incantation; otherwise update the comment.

@@ +180,5 @@
> +  } while (0)
> +#else
> +#define MOZ_CRASH() \
> +  do { \
> +      raise(SIGABRT);  /* To continue from here in GDB: "signal 0". */ \

Ditto for this comment re updating it or filing a bug to update it (you can probably file one bug for both cases).

@@ +189,5 @@
>  extern MFBT_API(void)
>  MOZ_Assert(const char* s, const char* file, int ln);
>  
> +MOZ_ALWAYS_INLINE void
> +OutputAssertMessage(const char* s, const char *file, int ln)

This seems to me like it should be an MFBT_API() function defined in Assertions.cpp and declared here, to be called by the assert macro.  I don't see the gain to force-inlining it.

The function body is over-indented, should be two spaces, not four.

@@ +240,5 @@
>  #ifdef DEBUG
>     /* First the single-argument form. */
>  #  define MOZ_ASSERT_HELPER1(expr) \
> +   do { \
> +       if ((expr)) { \

Can you think of anything that would go wrong if we were to have |if (expr)| here?  The only problem I can imagine would be if there were unbalanced parentheses.  But 1) is that even possible? and 2) it would seem to require some malice if it is is somehow possible.  So to make bz's macro-warning case nicer, we should probably remove the redundant parentheses here, and in all the other #defines.

@@ +295,5 @@
>   */
>  #ifdef DEBUG
> +#  define MOZ_ASSERT_IF(cond, expr) \
> +    do { \
> +        if ((cond)) MOZ_ASSERT(expr); \

Split this across two lines, please.

@@ +301,2 @@
>  #else
>  #  define MOZ_ASSERT_IF(cond, expr)  ((void)0)

Convert this to |do { } while (0)| so that it has the same contextual requirements as #ifdef DEBUG.

Comment 20

[Bas Schouten (:bas.schouten)]

Comment on attachment 613798 [details] [diff] [review]
Part 2: Inline MOZ_Assert and MOZ_Crash

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

::: mfbt/Assertions.h
@@ +174,5 @@
> +   * trapped.
> +   */
> +#define MOZ_CRASH() \
> +  do { \
> +      *((volatile int *) NULL) = 123;  /* To continue from here in GDB: "return" then "continue". */ \

Filed bug 744283.

@@ +180,5 @@
> +  } while (0)
> +#else
> +#define MOZ_CRASH() \
> +  do { \
> +      raise(SIGABRT);  /* To continue from here in GDB: "signal 0". */ \

I believe this one is still valid.

Comment 21

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Inline MOZ_Assert and MOZ_Crash v2

As per comments and IRC. I hope I got everything, I also made the release version of MOZ_ASSERT have the same contextual requirements.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 613838 [details] [diff] [review]
Part 2: Inline MOZ_Assert and MOZ_Crash v2

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

Some minor formatting nits, but few enough of them that I'm good with this.

::: mfbt/Assertions.h
@@ +48,5 @@
>  
> +#include <stdio.h>
> +#include <stdlib.h>
> +#ifndef WIN32
> +# include <signal.h>

#ifndef WIN32
#  include <signal.h>
#endif

In other words, add one more space before "include".

@@ +51,5 @@
> +#ifndef WIN32
> +# include <signal.h>
> +#endif
> +#ifdef ANDROID
> +# include <android/log.h>

Same here.

@@ +148,5 @@
> +#  define MOZ_CRASH() \
> +    do { \
> +      *((volatile int *) NULL) = 123; \
> +      exit(3); \
> +    } while (0)

The |do...while (0)| bits should be indented one more space, so that the "d" in "do" is two columns left of the start of "define", i.e. the "d" hangs underneath the "f".  Likewise for all the other macro bodies.

@@ +181,5 @@
>  extern MFBT_API(void)
>  MOZ_Assert(const char* s, const char* file, int ln);
>  
> +static MOZ_ALWAYS_INLINE void
> +OutputAssertMessage(const char* s, const char *file, int ln)

Could you make this MOZ_OutputAssertMessage for now, so that this is at least minimally namespaced, since all the other names we expose are namespaced too?

@@ +185,5 @@
> +OutputAssertMessage(const char* s, const char *file, int ln)
> +{
> +#ifdef ANDROID
> +    __android_log_print(ANDROID_LOG_FATAL, "MOZ_Assert",
> +                        "Assertion failure: %s, at %s:%d\n", s, file, ln);

This should only be indented two spaces.

@@ +188,5 @@
> +    __android_log_print(ANDROID_LOG_FATAL, "MOZ_Assert",
> +                        "Assertion failure: %s, at %s:%d\n", s, file, ln);
> +#else
> +    fprintf(stderr, "Assertion failure: %s, at %s:%d\n", s, file, ln);
> +    fflush(stderr);

These should only be indented two spaces.

@@ +237,5 @@
> +        OutputAssertMessage(#expr, __FILE__, __LINE__); \
> +        MOZ_CRASH(); \
> +      } \
> +    } while (0)
> +

No blank line here.

@@ +246,5 @@
> +        OutputAssertMessage(#expr " (" explain ")", __FILE__, __LINE__); \
> +        MOZ_CRASH(); \
> +      } \
> +    } while (0)
> +

Nor here.

@@ +289,5 @@
> +#  define MOZ_ASSERT_IF(cond, expr) \
> +    do { \
> +      if ((cond)) { \
> +        MOZ_ASSERT(expr); \
> +      } \

Single-line ifs aren't braced in mfbt code.

Comment 23

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Inline MOZ_Assert and MOZ_Crash v3

Updated as the last patch caused build failures and assertions weren't working right - that'll teach me to use TryServer.

The biggest issue is we have a places where ASSERT is used but there's a locally namespaced abort function. I added an #ifdef __cplusplus special case for android where we call abort(), but possibly we should do the same for raise and signal, I'm undecided.

Let me know what you think!

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 614032 [details] [diff] [review]
Part 2: Inline MOZ_Assert and MOZ_Crash v3

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

Let's keep this as simple as we can and introduce that complexity when it's needed.  It'll be easy to make more tweaks for raise/signal if necessary, but it's more readable without, so best avoided if possible.

Comment 25

[Mozilla RelEng Bot]

Try run for f50d262f22f4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f50d262f22f4
Results (out of 221 total builds):
    exception: 1
    success: 160
    warnings: 60
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f50d262f22f4

Comment 26

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 613763 [details] [diff] [review]
Part 3: Fix Azure stand-alone build.

Commit separate parts separately

Comment 27

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20218c1c79e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a964ce19e78f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f27e0a7e5809
https://hg.mozilla.org/integration/mozilla-inbound/rev/56aa3326470c

Comment 28

[(no longer active)]

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

Comment 29

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/4b82f26f8593

Comment 30

[(no longer active)]

Backed out: Backed out: https://hg.mozilla.org/mozilla-central/rev/ed7531e39066

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/20218c1c79e0
https://hg.mozilla.org/mozilla-central/rev/a964ce19e78f
https://hg.mozilla.org/mozilla-central/rev/f27e0a7e5809
https://hg.mozilla.org/mozilla-central/rev/56aa3326470c