Expose nsIThreadManager SetIgnoreThreadStatus API

RESOLVED WONTFIX

Status

defect
P1
normal
RESOLVED WONTFIX
4 years ago
4 years ago

People

(Reporter: anshulj, Assigned: cyu)

Tracking

unspecified
2.2 S11 (1may)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.2 unaffected, b2g-master unaffected)

Details

(Whiteboard: [caf priority: p2][CR 802467])

Attachments

(1 attachment, 13 obsolete attachments)

3.21 KB, patch
cyu
: review+
Details | Diff | Splinter Review
In bug 970307 an API SetIgnoreThreadStatus was introduced to allow a thread to tell nsThreadStatusManagerto ignore its status. This API is currently internal only due to which extensions like QC RIL and location are unable to set the thread status of ignore indirectly causing the apps to take longer to launch.
Whiteboard: [CR 802467]
Whiteboard: [CR 802467] → [caf priority: p2][CR 802467]
Since preload script won't use extensions, exposing method of setting ignore should be enough.
Assignee: nobody → kk1fff
Hey Patrick, here's a temporary patch that resolved the issue for us.  Hopefully this will be useful in writing something that can be landed in Gecko proper -- https://www.codeaurora.org/cgit/quic/lf/b2g/build/commit/?h=v2.2&id=5c61667b07e65b974d42693b436149451229e8ef
(In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> Hey Patrick, here's a temporary patch that resolved the issue for us. 
> Hopefully this will be useful in writing something that can be landed in
> Gecko proper --
> https://www.codeaurora.org/cgit/quic/lf/b2g/build/commit/?h=v2.
> 2&id=5c61667b07e65b974d42693b436149451229e8ef

Thanks, Michael! It looks like I am doing in the same way. :)
Status: NEW → ASSIGNED
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8572453 [details] [diff] [review]
Expose SetIgnoreThreadStatus() to XPCOM

However, I think maybe the right solution might be finding a way to exclude extension threads by default..
Attachment #8572453 - Flags: review?(nfroyd)
Anshul, how is the thread in QC RIL created? I'd like to know why is the nsThreadStatusInfo created for that thread.
Flags: needinfo?(anshulj)
Patrick, we are creating a new thread via NS_NewThread API and then pass it a runnable to run.
Flags: needinfo?(anshulj)
No longer blocks: CAF-v3.0-FL-metabug
Patrick,

Now that Anshul's responded, what's next? Do you now have the info you need to move ahead?

Thanks,
Mike
Flags: needinfo?(kk1fff)
(In reply to Mike Lee [:mlee] from comment #8)
> Patrick,
> 
> Now that Anshul's responded, what's next? Do you now have the info you need
> to move ahead?
> 
> Thanks,
> Mike

Yeah, I think I got what the root cause is from Anshul's response. But still working out a way to fix this properly.
Flags: needinfo?(kk1fff)
Thanks Patrick.

Do you now have a plan for fixing this? How much time do you anticipate needing to create that plan and deliver a fix?

We (CAF & Mozilla) are triaging all fxOS 2.2 FC blockers at least weekly and will needinfo each bug that hasn't been updated with progress information within that time.

Mike
Flags: needinfo?(kk1fff)
I am focusing on bug 1053634 recently. I think it would take a week to write a fix for this bug since I start working in this bug, since this will change the way threads report that they want to be ignored.
Flags: needinfo?(kk1fff)
Thinker,

Since Patrick is occupied with the critical bug 1053634 please have someone else on your team take a look at this as it is critical to improving the app launch times. Current feedback from CAF is that app launch times are significantly higher and not meeting the established goal without the temporary patch (comment 2) CAF has applied.

Thanks,
Mike
Flags: needinfo?(tlee)
According offline discussion with Patrick, I suggest that QC set a thread name for their thread.  And, we build a list of ignored thread in a preference and read it at runtime.

cervantes, could you take a look?
Flags: needinfo?(tlee) → needinfo?(cyu)
I can take a look.
Flags: needinfo?(cyu)
I'll upload my WIP for your reference.
Posted patch wip (obsolete) — Splinter Review
I end up with using observer service to pass the thread id, since it's a bit hard to give all ignored thread a name. The problem of this patch is that I haven't found a timing to set up observer before any other thread created.
Attachment #8572453 - Attachment is obsolete: true
Hi Cervantes,

Do you have an update about this? How are you progressing with this? Should you be set as the new assignee here?

Thanks,
Mike
Flags: needinfo?(cyu)
(In reply to Thinker Li [:sinker] from comment #13)
> According offline discussion with Patrick, I suggest that QC set a thread
> name for their thread.  And, we build a list of ignored thread in a
> preference and read it at runtime.
> 
> cervantes, could you take a look?
Thinker, how do you propose we set a thread name?
(In reply to Mike Lee [:mlee] from comment #17)
> Hi Cervantes,
> 
> Do you have an update about this? How are you progressing with this? Should
> you be set as the new assignee here?
> 
> Thanks,
> Mike

I am picking up the context for this bug. Having observer service for external modules and NS_SetIgnoreStatusOfCurrentThread() for internal code doesn't look good to me. I am working a new solution and will update soon.
Assignee: kk1fff → cyu
Flags: needinfo?(cyu)
Posted patch WIP (obsolete) — Splinter Review
WIP that uses a pref.
Attachment #8584563 - Attachment is obsolete: true
This allows external modules to create named threads that are ignored by default for thread status monitoring. To do this, specify the thread names of the ignored threads using the pref "dom.ipc.ignoreThreadStatus.threadNames".
Attachment #8587381 - Attachment is obsolete: true
Attachment #8589021 - Flags: review?(nfroyd)
Comment on attachment 8589021 [details] [diff] [review]
Allow external modules to create unmonitored threads for stablization of the Nuwa process.

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

I agree that comment 16's patch isn't a great way to go about things, but I'm curious why the original patch from comment 2 and comment 3 was discarded.  Comment 5 isn't much of an explanation, and it's not obvious to me why adding a string to an undocumented pref is any better that requiring extension authors to make an extra function call.  Comment 13 hints at some discussion as to why the current approach was chosen; could that discussion be repeated here?

Or, alternatively, we could have NS_NewIgnoredThread(), MOZ_NUWA_PROCESS-only.

::: xpcom/threads/nsThreadManager.cpp
@@ +338,5 @@
> +  // Thread name can be changed any time. Re-get thread name.
> +  thrInfo->mThreadName = PR_GetThreadName(PR_GetCurrentThread());
> +  if (!thrInfo->mThreadName) {
> +    return thrInfo;
> +  }

Surely we could at least check whether the thread name we get from PR_GetThreadName is the same as the one we already have.  Pointer equality should work, I think, and that would enable bypassing the expensive Find() call below.

@@ +600,5 @@
> +    Preferences::GetString("dom.ipc.ignoreThreadStatus.threadNames");
> +
> +  for (size_t i = 0; i < mThreadStatusInfos.Length(); i++) {
> +    ThreadStatusInfo* info = mThreadStatusInfos[i];
> +    if (info->mThreadName &&

Might as well check for !info->mIgnored.
Attachment #8589021 - Flags: review?(nfroyd)
Also please note that there are multiple independent components that create threads, so now they'll need to coordinate to set this undocumented pref properly?  That doesn't sound great.
I discarded the first patch because that relationship between making a new thread and calling ignore thread after the new thread created is not obvious. It is okay for Mozilla code, but if we expose the function to extensions, programmers would have to make an extra function call because of our internal mechanism. Therefore I tried to fix this with a little more decouple way, so if one day we changed the mechanism, we can remove/change that a little bit easier. (Won't make extensions rebuilt, at least)
Patrick, I have a concern in general about the original design of Nuwa waiting for all threads to go Idle and is that any extension that has a thread that doesn't go idle can impact the start up performance of each and every app on the phone, which doesn't seem ideal at all from an OS point of view.
(In reply to Anshul from comment #25)
> Patrick, I have a concern in general about the original design of Nuwa
> waiting for all threads to go Idle and is that any extension that has a
> thread that doesn't go idle can impact the start up performance of each and
> every app on the phone, which doesn't seem ideal at all from an OS point of
> view.

That's true. That is what we should fix. I think the ideal way to fix this is to handle the status of threads from extension automatically, or to ignore threads by default and care about only threads that reports their status. However, if threads are ignored by default when they were created, there could be a short period between "end of the task that creates the new thread" and "new thread begins to report status", in which we might fire a bogus event to make nuwa ready, which might cause deadlock randomly.

So I guess fixing with a better solution would take more time, and if we are providing a temporary solution, it might be better if we keep it easier to change in the future.
If we're providing a temporary solution to unblock v2.2 then let's use what already exists and is working.  That is, https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/patch/all/gecko/bug-1138620-Expose-SetIgnoreThreadStatus-API.patch?h=v2.2

For m-c, we have all the time in the world to produce the ideal alternative and when available we'll make the necessary adjustments over here from the v2.2 solution.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #23)
> Also please note that there are multiple independent components that create
> threads, so now they'll need to coordinate to set this undocumented pref
> properly?  That doesn't sound great.

Multiple independent modules creating threads is also supported. Just add all the thread names to the pref value such as:

dom.ipc.ignoreThreadStatus.threadNames=Thread1,Thread2,Thread3.

The pref is not designed to be used as an observer, where one module sets the pref value and the observer gets notified and acts accordingly. The pref value needs to be specified in build stage.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #22)
> Comment on attachment 8589021 [details] [diff] [review]
> Allow external modules to create unmonitored threads for stablization of the
> Nuwa process.
> 
> Review of attachment 8589021 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree that comment 16's patch isn't a great way to go about things, but
> I'm curious why the original patch from comment 2 and comment 3 was
> discarded.  Comment 5 isn't much of an explanation, and it's not obvious to
> me why adding a string to an undocumented pref is any better that requiring
> extension authors to make an extra function call.  Comment 13 hints at some
> discussion as to why the current approach was chosen; could that discussion
> be repeated here?
> 

The rationale behind the usage of a pref is that it has looser coupling between MOZILLA_INTERNAL code and external modules. Suppose an external module needs to create ignored (named) threads, it only needs to specify the thread names using the pref. No extra function call needs to be made to exclude the thread from thread status monitoring, and the pref can be added in build time. It better hides the implementation detail to external modules. It's also easier for us to make changes to thread status monitoring: if we have a better way for thread status monitoring without explicitly make function calls to ignore the threads, we can change the design without breaking existing modules. The pref just becomes unattended. No extra effort needs to be made.

The bad about using the pref is that it's less obvious than the approach in comment #2 and #3. It's harder to see how the pref gets to work. If you prefer the simpler design to the looser coupling, I am inclined to see XPCOM approach land in gecko.
(In reply to Cervantes Yu from comment #29)
> If you prefer the simpler design to the looser coupling, I am inclined to see XPCOM
> approach land in gecko.

Yes, I do.  See comment 27.  The coordination (build-time or otherwise) of keeping a global pref correct between otherwise independent XPCOM extensions is very undesirable.
Since I'm not up to speed on B2G building, what are these extensions, exactly?  Folks are building binary XPCOM components that are being loading to implement particular contract IDs?
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #31)
> Since I'm not up to speed on B2G building, what are these extensions,
> exactly?  Folks are building binary XPCOM components that are being loading
> to implement particular contract IDs?

These extensions are binary xpcom provided by some partners to implement radio & geolocation features.
(In reply to Fabrice Desré [:fabrice] from comment #32)
> These extensions are binary xpcom provided by some partners to implement
> radio & geolocation features.

OK, thanks.

Do we think of these binary xpcom extensions as part of the build (i.e. tied to a particular version of libxul) or do we think of them as mostly-independent from the libxul version?  If it's the former case, then I don't see a problem tying them to internal XPCOM details, since folks will be building them anew when libxul updates (and fixing bustage, etc.).  If it's the latter, then some sort of looser coupling seems appropriate, as we would (theoretically) like the ability to move libxul ahead some number of versions while not recompiling the binary extensions.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #33)
> Do we think of these binary xpcom extensions as part of the build (i.e. tied
> to a particular version of libxul) 

Yes, they are very tightly coupled to a specific libxul branch/version (v2.2 in this case).
What's the v2.2 plan for this bug?  See comment 27.
Flags: needinfo?(cyu)
Are you suggesting that we land the patch in comment #27 in 2.2 only while working on a different solution on m-c? It doesn't sound good provided that m-c currently also needs the functionality.

If binary dependency is not a concern, I would rather see a #ifdef MOZ_NUWA_PROCESS only API, NS_NewUnmonitoredThread(), which is equivalent to NS_NewThread(), but the thread is ignored by default. This is much clearer than setting the thread status through nsIThreadStatusManager that the caller is pretty sure of what it is doing.
Flags: needinfo?(cyu)
The proposal of NS_NewUnmonitoredThread() API. I think this is better than adding a method to nsIThreadManager because this API is less error-prone and the intention is clearer.
(In reply to Cervantes Yu from comment #36)
> Are you suggesting that we land the patch in comment #27 in 2.2 only while
> working on a different solution on m-c? It doesn't sound good provided that
> m-c currently also needs the functionality.

Definitely not.  Just that we land something that works for v2.2 and if we want to refine the m-c solution later then that's just fine.

> If binary dependency is not a concern, I would rather see a #ifdef
> MOZ_NUWA_PROCESS only API, NS_NewUnmonitoredThread(), which is equivalent to
> NS_NewThread(), but the thread is ignored by default.

Sounds good, binary dependency is not a concern right now.  Fixing this bug sooner rather than later is though.
Attachment #8589021 - Attachment is obsolete: true
Attachment #8591623 - Attachment is obsolete: true
Attachment #8592184 - Flags: review?(nfroyd)
(Unfinished) Test case for NS_NewUnmonitoredThread() that crashes, but shows how I am going to test it.
Comment on attachment 8592184 [details] [diff] [review]
NS_NewUnmonitoredThread() for Nuwa process stabalization

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

A few things to fix and one question below.

::: xpcom/glue/nsThreadUtils.cpp
@@ +111,5 @@
> +NS_IMETHODIMP IgnoreThreadStatusRunnable::Run(void)
> +{
> +#ifdef MOZILLA_INTERNAL_API
> +  nsThreadManager::get()->SetIgnoreThreadStatus();
> +#endif

I think it'd be prudent to have a:

#else
/* fail loudly in some way */
#endif

here instead.

@@ +130,5 @@
> +  }
> +
> +  nsCOMPtr<nsIRunnable> ignoreme = new IgnoreThreadStatusRunnable();
> +  rv = thread->Dispatch(ignoreme, NS_DISPATCH_NORMAL);
> +  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to set thread status ignored");

Nit: "Failed to set thread status to ignored".

::: xpcom/glue/nsThreadUtils.h
@@ +60,5 @@
>               nsIRunnable* aInitialEvent = nullptr,
>               uint32_t aStackSize = nsIThreadManager::DEFAULT_STACK_SIZE);
>  
> +#ifdef MOZ_NUWA_PROCESS
> +extern NS_METHOD

Nit: this should be moved down below the block comment.

Non-nit: I don't have a B2G build available, but I see that NS_NewThread is not exported from libxul in my x86-64 Linux build.  Since this method copies NS_NewThread, I assume that it also does not get exported from libxul.  But comment 7 suggests that these extensions are already using NS_NewThread (from outside libxul, I assume?).  And so I guess they'd be able to use NS_NewUnmonitoredThread as well?  But how does that work?  Do we compile libxul with different symbol visibility on B2G?

@@ +70,5 @@
> +                        nsIRunnable* aInitialEvent = nullptr,
> +                        uint32_t aStackSize =
> +                                 nsIThreadManager::DEFAULT_STACK_SIZE);
> +#else
> +#define NS_NewUnmonitoredThread(args...) NS_NewThread(args)

I think this is variable-argument template syntax, not variable-argument macro syntax.
Attachment #8592184 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #41)
> Non-nit: I don't have a B2G build available, but I see that NS_NewThread is
> not exported from libxul in my x86-64 Linux build.  Since this method copies
> NS_NewThread, I assume that it also does not get exported from libxul.  But
> comment 7 suggests that these extensions are already using NS_NewThread
> (from outside libxul, I assume?).  And so I guess they'd be able to use
> NS_NewUnmonitoredThread as well?  But how does that work?  Do we compile
> libxul with different symbol visibility on B2G?

Yes, I suppose NS_NewUnmonitoredThread() is visible to the module in question since it uses NS_NewThread() to create a thread. Maybe it's not external to libxul.so and is built as part of it? Mike, would you clarify this?
Flags: needinfo?(mvines)
The components that need NS_NewUnmonitoredThread() are built outside of the Gecko build system/tree and know nothing of internal mozbuild configuration defines like MOZ_NUWA_PROCESS, so when nsThreadUtils.h is included they will see:

  #define NS_NewUnmonitoredThread(args...) NS_NewThread(args)

with attachment 8592184 [details] [diff] [review].

The implementations of NS_New*Thread() end up in ./obj/objdir-gecko/dist/lib/libxpcomglue_s.a and statically linked with the components, so that's why these symbols are not visible as exports from libxul.so
Flags: needinfo?(mvines)
Ah, yes, all the different glue compilation variations.  So this patch is not going to work if those consumers can't see the correct version of NS_NewUnmonitoredThread.
The main change is that NS_NewUnmonitoredThread() is always visible in the header file.
Also fix the nits and use threadsafe nsISupport in the runnable.
Attachment #8592184 - Attachment is obsolete: true
Attachment #8593339 - Flags: review?(nfroyd)
The test case for NS_NewUnmonitoredThread().
Attachment #8592186 - Attachment is obsolete: true
Attachment #8593340 - Flags: review?(nfroyd)
Comment on attachment 8593340 [details] [diff] [review]
test_new_unmonitoredthread.patch

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

Um, I'm not the right reviewer for this.  Bugzilla says Fabrice might be?
Attachment #8593340 - Flags: review?(nfroyd) → review?(fabrice)
Comment on attachment 8593339 [details] [diff] [review]
Allow binary modules to create unmonitored threads that doesn't keep the Nuwa process from stablization

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

Sad that we have to expose this to non-Nuwa, but that's the way things go...

r=me with the changes below.

::: xpcom/glue/nsThreadUtils.cpp
@@ +134,5 @@
> +  }
> +
> +  nsCOMPtr<nsIRunnable> ignoreme = new IgnoreThreadStatusRunnable();
> +  rv = thread->Dispatch(ignoreme, NS_DISPATCH_NORMAL);
> +  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to set thread status to ignored");

Seems like you should return rv here if NS_FAILED(rv).  I know that you return rv below, but trying to dispatch another event when this one has failed doesn't seem like a good strategy.

@@ +138,5 @@
> +  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to set thread status to ignored");
> +
> +  if (aEvent) {
> +    rv = thread->Dispatch(aEvent, NS_DISPATCH_NORMAL);
> +    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Initial event dispatch failed");

Same here.

@@ +143,5 @@
> +  }
> +
> +  *aResult = nullptr;
> +  thread.swap(*aResult);
> +  return rv;

Also, returning a thread *and* returning a possibly non-NS_OK nsresult is kind of unusual.  Better to just return early and not give callers a thread that hasn't been setup properly rather than give them a thread and the opportunity to completely ignore that things went wrong.

::: xpcom/glue/nsThreadUtils.h
@@ +62,5 @@
>  
>  /**
> + * Create a new thread that is ignored in thread status monitoring by default on
> + * platforms with Nuwa process enabled. On non-Nuwa platforms, this function is
> + * identical to Ns_NewThread().

Nit: NS_NewThread
Attachment #8593339 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #48)
> Comment on attachment 8593340 [details] [diff] [review]
> test_new_unmonitoredthread.patch
> 
> Review of attachment 8593340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Um, I'm not the right reviewer for this.  Bugzilla says Fabrice might be?

Bugzilla is not smart enough here, or we should file that somewhere else. Redirecting to Kyle since he actually knows something about nuwa.
Attachment #8593340 - Flags: review?(fabrice) → review?(khuey)
Carry over r+ from :nfroyd.
Attachment #8593339 - Attachment is obsolete: true
Attachment #8593815 - Flags: review+
Hi, Kyle, since this bug is in CAF bug list and it was marked as blocking-b2g:2.2+, could you help to review it with higher priority? Thank you very much.
Flags: needinfo?(khuey)
Comment on attachment 8593340 [details] [diff] [review]
test_new_unmonitoredthread.patch

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

It does not appear that the monitor actually needs to be reentrant.
Attachment #8593340 - Flags: review?(khuey) → review+
Update: use mozilla::Monitor in the test case. Carry over r+ from :khuey.
Attachment #8593340 - Attachment is obsolete: true
Attachment #8595276 - Flags: review+
Comment on attachment 8593815 [details] [diff] [review]
Allow binary modules to create unmonitored threads that doesn't keep the Nuwa process from stablization

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 970307
User impact if declined: If partner code that create a thread for its use could experience longer app startup time and higher memory usage. This patch opens a new API method for partner to create a thread without the problem.
Testing completed: Automated test cases. Manually tested on a device.
Risk to taking this patch (and alternatives if risky): Low. No existing code in gecko is using the added method.
Attachment #8593815 - Flags: approval-mozilla-b2g37?
Comment on attachment 8595276 [details] [diff] [review]
Test case for New_UnmonitoredThread()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 970307
User impact if declined: None because this is a test case for the patch.
Testing completed: Mochitest run locally and pushed to try.
Risk to taking this patch (and alternatives if risky): Low. This is a test case for the patch.
Attachment #8595276 - Flags: approval-mozilla-b2g37?
Hey Anshul, does the latest patch for this bug work over here?
Flags: needinfo?(anshulj)
Attachment #8593815 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8595276 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/mozilla-central/rev/52d49dd25d6d
https://hg.mozilla.org/mozilla-central/rev/e60b4b01c05d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
Comment on attachment 8593815 [details] [diff] [review]
Allow binary modules to create unmonitored threads that doesn't keep the Nuwa process from stablization

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

This patch doesn't work as call to nsThreadManager::get()->SetIgnoreThreadStatus() inside IgnoreThreadStatusRunnable is featurized by MOZILLA_INTERNAL_API flag that is not defined in our builds. So the patch does nothing in our builds and the original issue remains.
Attachment #8593815 - Flags: feedback-
Flags: needinfo?(anshulj)
Reopning the bug as the issue is not resolved. Following is the error I get when I remove #ifdef MOZILLA_INTERNAL_API at https://hg.mozilla.org/mozilla-central/rev/52d49dd25d6d#l1.29.

gecko/xpcom/glue/nsThreadUtils.cpp:113:3: error: 'nsThreadManager' has not been declared
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hi! Cervantes,

Could you help to take a look? Thanks

--
Keven
Flags: needinfo?(cyu)
(In reply to Anshul from comment #61)
> Comment on attachment 8593815 [details] [diff] [review]
> Allow binary modules to create unmonitored threads that doesn't keep the
> Nuwa process from stablization
> 
> Review of attachment 8593815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch doesn't work as call to
> nsThreadManager::get()->SetIgnoreThreadStatus() inside
> IgnoreThreadStatusRunnable is featurized by MOZILLA_INTERNAL_API flag that
> is not defined in our builds. So the patch does nothing in our builds and
> the original issue remains.

I finally figured out what happens after wrestling with the XPCOM glue stuff. It's painful that we don't have a way to test whether the patch really works on our side.

Since your module is not linked with libxpcomglue instead of libxul, it calls NS_NewUnmonitoredThread() through the stub version, which is directed to NS_NewThread(). Since the module accesses the internal function through the glue, we can only:

* Keep NS_NewUnmonitoredThread() as is, but add nsIThreadManager::SetIgnoreThreadStatus() as in comment #27. The implementation of NS_NewUnmonitoredThread() calls nsIThreadManager::SetIgnoreThreadStatus(), but doing this doesn't isn't any better than having comment #27 alone. Anyway nsIThreadManager::SetIgnoreThreadStatus() is exposed.

* Just backout the patches and take comment #27 instead.

* NS_EXPORT the function NS_NewUnmonitoredThread(). The binary module accesses it using a weak symbol.

What do you say?
Flags: needinfo?(cyu)
Flags: needinfo?(nfroyd)
Flags: needinfo?(mvines)
(In reply to Cervantes Yu from comment #64)
> Since your module is not linked with libxpcomglue instead of libxul, it
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                    is linked with libxpcomglue instead of libxul
(In reply to Cervantes Yu from comment #64)
> 
> I finally figured out what happens after wrestling with the XPCOM glue
> stuff. It's painful that we don't have a way to test whether the patch
> really works on our side.

We are just an ni? away and will happily try out WIPs anytime!

> * Just backout the patches and take comment #27 instead.
> 
> * NS_EXPORT the function NS_NewUnmonitoredThread(). The binary module
> accesses it using a weak symbol.
> 
> What do you say?

My vote remains on comment 27 because it actually works today and is shipping, and all other solutions discussed so far here don't really improve it in any meaningful way.

Oh, and the v2.2 clock is still ticking.
Flags: needinfo?(mvines)
(In reply to Cervantes Yu from comment #64)
> (In reply to Anshul from comment #61)
> > This patch doesn't work as call to
> > nsThreadManager::get()->SetIgnoreThreadStatus() inside
> > IgnoreThreadStatusRunnable is featurized by MOZILLA_INTERNAL_API flag that
> > is not defined in our builds. So the patch does nothing in our builds and
> > the original issue remains.
> 
> I finally figured out what happens after wrestling with the XPCOM glue
> stuff. It's painful that we don't have a way to test whether the patch
> really works on our side.

Sorry, I should have thought about the review a little harder and what went where in the glue.

> * Keep NS_NewUnmonitoredThread() as is, but add
> nsIThreadManager::SetIgnoreThreadStatus() as in comment #27. The
> implementation of NS_NewUnmonitoredThread() calls
> nsIThreadManager::SetIgnoreThreadStatus(), but doing this doesn't isn't any
> better than having comment #27 alone. Anyway
> nsIThreadManager::SetIgnoreThreadStatus() is exposed.

I'd rather not expose two different ways of doing this.

> * Just backout the patches and take comment #27 instead.

That patch is OK as a get-it-to-work patch, but I think it needs a little bit more work (comments, probably not exporting the new method to script, etc.) before landing.  (Not opposed to the approach, just that it won't land as-is.)

> * NS_EXPORT the function NS_NewUnmonitoredThread(). The binary module
> accesses it using a weak symbol.

I'd rather not export more things like this if we can avoid it.

Comment 43 says that these binary components are built outside of the Mozilla tree, and so don't see MOZ_NUWA_PROCESS etc....but don't these binary components include mozilla-config.h?  That *does* have a #define for MOZ_NUWA_PROCESS, and probably a lot of other things...I'd be a little surprised that headers work without bringing mozilla-config.h in somewhere.  Can't we just build the binary components with mozilla-config.h included, and then we can use the previous iteration of NS_NewUnmonitoredThread()?
Flags: needinfo?(nfroyd) → needinfo?(mvines)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #67)
> (In reply to Cervantes Yu from comment #64)
>
> > * Just backout the patches and take comment #27 instead.
> 
> That patch is OK as a get-it-to-work patch, but I think it needs a little
> bit more work (comments, probably not exporting the new method to script,
> etc.) before landing.  (Not opposed to the approach, just that it won't land
> as-is.)

Yep, for sure.  

> 
> > * NS_EXPORT the function NS_NewUnmonitoredThread(). The binary module
> > accesses it using a weak symbol.
> 
> I'd rather not export more things like this if we can avoid it.
> 
> Comment 43 says that these binary components are built outside of the
> Mozilla tree, and so don't see MOZ_NUWA_PROCESS etc....but don't these
> binary components include mozilla-config.h?  That *does* have a #define for
> MOZ_NUWA_PROCESS, and probably a lot of other things...I'd be a little
> surprised that headers work without bringing mozilla-config.h in somewhere. 
> Can't we just build the binary components with mozilla-config.h included,
> and then we can use the previous iteration of NS_NewUnmonitoredThread()?

These components get built independently from Gecko after the Gecko build process completes.  So defining MOZ_NUWA_PROCESS, or including some new header that defines it, won't help if the symbol it declares isn't actually exported from libxul.

This may be helpful (or more confusing), but here's the Android build system goo used to build these components: 
* https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/clear_xpcom_vars.mk?h=v2.2
* https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/xpcom.mk?h=v2.2

Unfortunately there's no reference to an Android.mk using this goo that I can easily point you to for reference.  It looks just like a normal Android.mk setup though, dynamically linking to libnspr4, libxul, libmozglue and statically linking to libxpcomglue_s
Flags: needinfo?(mvines)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #68)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #67)
> > (In reply to Cervantes Yu from comment #64)
> > > * NS_EXPORT the function NS_NewUnmonitoredThread(). The binary module
> > > accesses it using a weak symbol.
> > 
> > I'd rather not export more things like this if we can avoid it.
> > 
> > Comment 43 says that these binary components are built outside of the
> > Mozilla tree, and so don't see MOZ_NUWA_PROCESS etc....but don't these
> > binary components include mozilla-config.h?  That *does* have a #define for
> > MOZ_NUWA_PROCESS, and probably a lot of other things...I'd be a little
> > surprised that headers work without bringing mozilla-config.h in somewhere. 
> > Can't we just build the binary components with mozilla-config.h included,
> > and then we can use the previous iteration of NS_NewUnmonitoredThread()?
> 
> These components get built independently from Gecko after the Gecko build
> process completes.  So defining MOZ_NUWA_PROCESS, or including some new
> header that defines it, won't help if the symbol it declares isn't actually
> exported from libxul.

Sure, but e.g. comment 43 said that putting things behind MOZ_NUWA_PROCESS in header files is useless to binary components, because MOZ_NUWA_PROCESS isn't defined.  But MOZ_NUWA_PROCESS *should* be defined, if the binary components were being built with mozilla-config.h included.

I guess that doesn't necessarily solve the xpcom glue being built without MOZ_INTERNAL_API or similar.
Yeah so there's surely another way to solve it and we could continue the search but then we're back to, does it really improve things over comment 27 given the lack of time remaining to pinch off v2.2.  Probably not.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #69)
> I guess that doesn't necessarily solve the xpcom glue being built without
> MOZ_INTERNAL_API or similar.

It doesn't. Whichever definition we add won't make it work because this is a linking issue. The binary module accesses NS_NewUnmonitoredThread() in libxul.so through NS_NewUnmonitoredThread() in libxpcomglue_s.a.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #60)
> https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a2188d1ce068
> https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1e676a9bb677

Ryan, would you help back these 2 changes out? Thanks.
Flags: needinfo?(ryanvm)
Attachment #8597068 - Flags: review?(nfroyd)
Comment on attachment 8597068 [details] [diff] [review]
nsIThreadManager::SetIgnoreThreadStatus() for external modules to create unmonitored threads that doesn't keep the Nuwa process from stablization

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

Thanks for adding the documentation.  r=me with the modification below.

::: xpcom/threads/nsIThreadManager.idl
@@ +67,5 @@
>    readonly attribute boolean isMainThread;
> +
> +  /**
> +   * Informs the thread manager to not monitor the status of the current thread.
> +   * The method is only used on platforms with Nuwa enabled.

Nit: "This method only works on platforms with Nuwa enabled; on other platforms, it throws an error."
Attachment #8597068 - Flags: review?(nfroyd) → review+
The checkin version of the patch. carry over r+ from :nfroyd.
Attachment #8597068 - Attachment is obsolete: true
Attachment #8597857 - Flags: review+
Comment on attachment 8597861 [details] [diff] [review]
nsIThreadManager::SetIgnoreThreadStatus() for external modules to create unmonitored threads that doesn't keep the Nuwa process from stablization

Bug caused by (feature/regressing bug #): 970307
User impact if declined: Partner code that create a thread for its use could result in longer app startup time and higher memory usage. This patch opens a new API method for partner to create a thread without the problem.
Testing completed: Manually.
Risk to taking this patch (and alternatives if risky): Low. No existing code in gecko is using the added method.
Attachment #8597861 - Flags: approval-mozilla-b2g37?
Attachment #8597861 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/mozilla-central/rev/b371db089894
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
Please note that we tend to back out bug 970307. See bug 1151672 for discussion. If 970307 is backed out, this bug needs to be backed out, too. The result is that the added API will no longer exist and also no longer needed. CAF needs to rebuild the binary module (we can leave an empty implementation to not break the external module if necessary, but doing this doesn't make much sense).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Pulsebot from comment #85)
> Backout:
> https://hg.mozilla.org/integration/b2g-inbound/rev/daa19810f688

As a result, this bug will be WONTFIX because the exposed API is unnecessary.
Per backout in comment 84 and 86, resolve as wontfix. Remove v2.2+ flag.
Status: REOPENED → RESOLVED
blocking-b2g: 2.2+ → ---
Closed: 4 years ago4 years ago
Resolution: --- → WONTFIX
(In reply to Cervantes Yu from comment #86)
> (In reply to Pulsebot from comment #85)
> > Backout:
> > https://hg.mozilla.org/integration/b2g-inbound/rev/daa19810f688
> 
> As a result, this bug will be WONTFIX because the exposed API is unnecessary.

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=614d52bca5ac since one of this changes broke ics debug tests with failures like :

https://treeherder.mozilla.org/logviewer.html#?job_id=1958358&repo=b2g-inbound
Flags: needinfo?(cyu)
Canceling NI flag. The bustage is fixed and the change is repushed.
Flags: needinfo?(cyu)
You need to log in before you can comment on or make changes to this bug.