Closed
Bug 1138620
Opened 9 years ago
Closed 9 years ago
Expose nsIThreadManager SetIgnoreThreadStatus API
Categories
(Firefox OS Graveyard :: General, defect, P1)
Tracking
(firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.2 unaffected, b2g-master unaffected)
RESOLVED
WONTFIX
2.2 S11 (1may)
Tracking | Status | |
---|---|---|
firefox38 | --- | wontfix |
firefox39 | --- | wontfix |
firefox40 | --- | fixed |
b2g-v2.2 | --- | unaffected |
b2g-master | --- | unaffected |
People
(Reporter: anshulj, Assigned: cyu)
References
Details
(Whiteboard: [caf priority: p2][CR 802467])
Attachments
(1 file, 13 obsolete files)
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.
Updated•9 years ago
|
Whiteboard: [CR 802467]
Updated•9 years ago
|
Whiteboard: [CR 802467] → [caf priority: p2][CR 802467]
Comment 1•9 years ago
|
||
Since preload script won't use extensions, exposing method of setting ignore should be enough.
Assignee: nobody → kk1fff
Comment 2•9 years ago
|
||
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
Comment 3•9 years ago
|
||
Attachment #8572453 -
Flags: review?(nfroyd)
Comment 4•9 years ago
|
||
(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. :)
Updated•9 years ago
|
Status: NEW → ASSIGNED
blocking-b2g: 2.2? → 2.2+
Comment 5•9 years ago
|
||
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)
Comment 6•9 years ago
|
||
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)
Blocks: CAF-v3.0-FL-metabug
No longer blocks: CAF-v3.0-FL-metabug
Comment 8•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
(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)
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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)
Comment 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
I'll upload my WIP for your reference.
Comment 16•9 years ago
|
||
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
Comment 17•9 years ago
|
||
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)
Reporter | ||
Comment 18•9 years ago
|
||
(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?
Assignee | ||
Comment 19•9 years ago
|
||
(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)
Assignee | ||
Comment 20•9 years ago
|
||
WIP that uses a pref.
Attachment #8584563 -
Attachment is obsolete: true
Assignee | ||
Comment 21•9 years ago
|
||
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 22•9 years ago
|
||
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)
Comment 23•9 years ago
|
||
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.
Comment 24•9 years ago
|
||
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)
Reporter | ||
Comment 25•9 years ago
|
||
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.
Comment 26•9 years ago
|
||
(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.
Comment 27•9 years ago
|
||
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.
Assignee | ||
Comment 28•9 years ago
|
||
(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.
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Comment 30•9 years ago
|
||
(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.
Comment 31•9 years ago
|
||
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?
Comment 32•9 years ago
|
||
(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.
Comment 33•9 years ago
|
||
(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.
Comment 34•9 years ago
|
||
(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).
Assignee | ||
Comment 36•9 years ago
|
||
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)
Assignee | ||
Comment 37•9 years ago
|
||
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.
Comment 38•9 years ago
|
||
(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.
Assignee | ||
Comment 39•9 years ago
|
||
Attachment #8589021 -
Attachment is obsolete: true
Attachment #8591623 -
Attachment is obsolete: true
Attachment #8592184 -
Flags: review?(nfroyd)
Assignee | ||
Comment 40•9 years ago
|
||
(Unfinished) Test case for NS_NewUnmonitoredThread() that crashes, but shows how I am going to test it.
Comment 41•9 years ago
|
||
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+
Assignee | ||
Comment 42•9 years ago
|
||
(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)
Comment 43•9 years ago
|
||
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)
Comment 44•9 years ago
|
||
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.
Assignee | ||
Comment 45•9 years ago
|
||
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)
Assignee | ||
Comment 46•9 years ago
|
||
The test case for NS_NewUnmonitoredThread().
Attachment #8592186 -
Attachment is obsolete: true
Attachment #8593340 -
Flags: review?(nfroyd)
Assignee | ||
Comment 47•9 years ago
|
||
Try push log: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db5b428efe3c
Comment 48•9 years ago
|
||
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 49•9 years ago
|
||
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+
Comment 50•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8593340 -
Flags: review?(fabrice) → review?(khuey)
Assignee | ||
Comment 51•9 years ago
|
||
Carry over r+ from :nfroyd.
Attachment #8593339 -
Attachment is obsolete: true
Attachment #8593815 -
Flags: review+
Comment 52•9 years ago
|
||
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+
Flags: needinfo?(khuey)
Assignee | ||
Comment 54•9 years ago
|
||
Update: use mozilla::Monitor in the test case. Carry over r+ from :khuey.
Attachment #8593340 -
Attachment is obsolete: true
Attachment #8595276 -
Flags: review+
Comment 55•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/52d49dd25d6d https://hg.mozilla.org/integration/b2g-inbound/rev/e60b4b01c05d
Assignee | ||
Comment 56•9 years ago
|
||
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?
Assignee | ||
Comment 57•9 years ago
|
||
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?
Comment 58•9 years ago
|
||
Hey Anshul, does the latest patch for this bug work over here?
Flags: needinfo?(anshulj)
Updated•9 years ago
|
Attachment #8593815 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•9 years ago
|
Attachment #8595276 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment 59•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/52d49dd25d6d https://hg.mozilla.org/mozilla-central/rev/e60b4b01c05d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
Comment 60•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a2188d1ce068 https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1e676a9bb677
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
status-firefox38:
--- → wontfix
status-firefox39:
--- → wontfix
Reporter | ||
Comment 61•9 years ago
|
||
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-
Reporter | ||
Comment 62•9 years ago
|
||
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 → ---
Comment 63•9 years ago
|
||
Hi! Cervantes, Could you help to take a look? Thanks -- Keven
Flags: needinfo?(cyu)
Assignee | ||
Comment 64•9 years ago
|
||
(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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(nfroyd)
Flags: needinfo?(mvines)
Assignee | ||
Comment 65•9 years ago
|
||
(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
Comment 66•9 years ago
|
||
(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)
Comment 67•9 years ago
|
||
(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)
Comment 68•9 years ago
|
||
(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)
Comment 69•9 years ago
|
||
(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.
Comment 70•9 years ago
|
||
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.
Assignee | ||
Comment 71•9 years ago
|
||
(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.
Assignee | ||
Comment 72•9 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba81575790fd
Assignee | ||
Comment 73•9 years ago
|
||
(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)
Assignee | ||
Comment 74•9 years ago
|
||
Attachment #8593815 -
Attachment is obsolete: true
Attachment #8595276 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8597068 -
Flags: review?(nfroyd)
Comment 75•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/41b2c33b2165
Flags: needinfo?(ryanvm)
Target Milestone: 2.2 S11 (1may) → ---
Comment 76•9 years ago
|
||
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+
Comment 77•9 years ago
|
||
(In reply to Cervantes Yu from comment #72) > Backed out: > https://hg.mozilla.org/integration/mozilla-inbound/rev/ba81575790fd Merge of backout: https://hg.mozilla.org/mozilla-central/rev/ba81575790fd
Assignee | ||
Comment 78•9 years ago
|
||
The checkin version of the patch. carry over r+ from :nfroyd.
Attachment #8597068 -
Attachment is obsolete: true
Attachment #8597857 -
Flags: review+
Assignee | ||
Comment 80•9 years ago
|
||
s/stabalization/stabilization in patch comment.
Attachment #8597857 -
Attachment is obsolete: true
Attachment #8597861 -
Flags: review+
Assignee | ||
Comment 81•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8597861 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment 82•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b371db089894
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
Assignee | ||
Comment 84•9 years ago
|
||
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).
Comment 85•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/b2g-inbound/rev/daa19810f688
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 86•9 years ago
|
||
(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.
Comment 87•9 years ago
|
||
Per backout in comment 84 and 86, resolve as wontfix. Remove v2.2+ flag.
Status: REOPENED → RESOLVED
blocking-b2g: 2.2+ → ---
Closed: 9 years ago → 9 years ago
Resolution: --- → WONTFIX
Comment 88•9 years ago
|
||
(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)
Comment 89•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/b2g-inbound/rev/305a36426c75
Assignee | ||
Comment 90•9 years ago
|
||
Canceling NI flag. The bustage is fixed and the change is repushed.
Flags: needinfo?(cyu)
Comment 91•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/b2g-inbound/rev/3385678d8fd5
You need to log in
before you can comment on or make changes to this bug.
Description
•