1410023 - Non-unified xpcom/threads build is failing

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Philippe Normand [:philn]]

Attachment: 2-Non_unified_build_fixes_for_xpcom_threads_.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13) AppleWebKit/604.1.38 (KHTML, like Gecko) Version/11.0 Safari/604.1.38



Actual results:

After the patches from bug 1404198 landed the non-unified build fails again in xpcom/threads.

Comment 1

[Philippe Normand [:philn]]

Attachment: 2-Bug_1410023___Non_unified_build_fixes_for_xpcom_threads_.patch

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8920082 [details] [diff] [review]
2-Bug_1410023___Non_unified_build_fixes_for_xpcom_threads_.patch

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

Thanks, but I have some questions about the nature of these fixes.

::: xpcom/threads/ThreadEventTarget.cpp
@@ +23,5 @@
>  
>  using namespace mozilla;
>  
> +nsresult
> +NS_NewTimerWithCallback(nsITimer** aTimer,

Why are we not simply including nsITimer.h for this declaration?

::: xpcom/threads/nsThreadUtils.cpp
@@ +81,5 @@
>  NS_IMPL_ISUPPORTS_INHERITED(IdleRunnable, CancelableRunnable,
>                              nsIIdleRunnable)
>  
> +nsresult
> +NS_NewTimerWithFuncCallback(nsITimer** aTimer,

Same question here.

::: xpcom/threads/nsThreadUtils.h
@@ +640,4 @@
>  namespace mozilla {
>  namespace detail {
>  
> +already_AddRefed<nsITimer> NS_NewTimer();

Same question here.

::: xpcom/threads/nsTimerImpl.cpp
@@ -74,5 @@
>    return timer.forget();
>  }
>  
> -mozilla::Result<nsCOMPtr<nsITimer>, nsresult>
> -NS_NewTimerWithObserver(nsIObserver* aObserver,

The diff here is very peculiar and I am unable to figure out what the intent of these changes was.  Can you shed some light on this, please?

Comment 3

[Philippe Normand [:philn]]

Comment on attachment 8920082 [details] [diff] [review]
2-Bug_1410023___Non_unified_build_fixes_for_xpcom_threads_.patch

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

Thanks for the review! Answers in-line.

::: xpcom/threads/ThreadEventTarget.cpp
@@ +23,5 @@
>  
>  using namespace mozilla;
>  
> +nsresult
> +NS_NewTimerWithCallback(nsITimer** aTimer,

Because nsITimer.h seems to be generated during the build and doesn't include this declaration.

::: xpcom/threads/nsThreadUtils.cpp
@@ +81,5 @@
>  NS_IMPL_ISUPPORTS_INHERITED(IdleRunnable, CancelableRunnable,
>                              nsIIdleRunnable)
>  
> +nsresult
> +NS_NewTimerWithFuncCallback(nsITimer** aTimer,

Same answer :)

::: xpcom/threads/nsThreadUtils.h
@@ +640,4 @@
>  namespace mozilla {
>  namespace detail {
>  
> +already_AddRefed<nsITimer> NS_NewTimer();

Same answer :)

::: xpcom/threads/nsTimerImpl.cpp
@@ -74,5 @@
>    return timer.forget();
>  }
>  
> -mozilla::Result<nsCOMPtr<nsITimer>, nsresult>
> -NS_NewTimerWithObserver(nsIObserver* aObserver,

Yes, I'm sorry about this. The diff is indeed confusing. The reason for these changes is that the function taking an nsIObserver as first argument call the variant of the same function but with a timer as first argument. But the latter isn't declared yet at that point. So I swapped the declaration order of each NS_NewTimer...() function to avoid this issue.

I had this specific issue on High Sierra with the XCode 9 toolchain.

Comment 4

[Philippe Normand [:philn]]

Oh I'm sorry, I didn't carefully check your comments about nsITimer.h. I'll try to update the patch!

Comment 5

[Philippe Normand [:philn]]

Attachment: 1-Bug_1410023___Non_unified_build_fixes_for_xpcom_threads_.patch

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8920616 [details] [diff] [review]
1-Bug_1410023___Non_unified_build_fixes_for_xpcom_threads_.patch

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

Thanks, this patch looks better!  Just canceling the review to address the issue below.

::: xpcom/threads/nsTimerImpl.cpp
@@ -80,5 @@
> -                        uint32_t aType,
> -                        nsIEventTarget* aTarget)
> -{
> -  nsCOMPtr<nsITimer> timer;
> -  MOZ_TRY(NS_NewTimerWithObserver(getter_AddRefs(timer),

Thanks for the explanation of what the changes to this file were supposed to be doing.  But I am confused why this function wouldn't be declared...it's declared in nsITimer.h, which is included by nsTimerImpl.h, which is included by this file.  So we shouldn't have any issues with the ordering here?

Comment 7

[Philippe Normand [:philn]]

Attachment: 2-Bug_1410023___Non_unified_build_fixes_for_xpcom_threads_.patch

Actually after checking the changes in this file again it turns out they're indeed un-necessary :) When I came up with this patch I was unknowingly relying on a old nsITimer.h header from gecko-media, that tricked me to this change!

Thanks for the thorough review, Nathan.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8920630 [details] [diff] [review]
2-Bug_1410023___Non_unified_build_fixes_for_xpcom_threads_.patch

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

Excellent.  Thank you for your patience!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/facc1bc84098
Non-unified build fixes for xpcom/threads. r=froydnj

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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