Closed Bug 1006336 Opened 6 years ago Closed 6 years ago

Don't use NS_GetCurrentThread in GLContext, which is not running on an nsThread

Categories

(Core :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: kk1fff, Assigned: pchang)

References

Details

Attachments

(1 file, 1 obsolete file)

GLContext is not running on an nsThread, calling NS_GetCurrentThread() will create a unused nsThread date structure.
Basicaaly, we can remove those mOwningThread codes since no one really use it beside a very simple thread checking. I think it would be a great first bug.
(:kk1fff) from comment #0)
> GLContext is not running on an nsThread, calling NS_GetCurrentThread() will
> create a unused nsThread date structure.

Current compositor thread will create the GLContext and it was created as a pthread.
Since no one is calling DispatchToOwningThread, I think we could remove nsThread related stuff from GLContext and get tid by PlatformThread::CurrentId().

http://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContext.cpp#1971
Assignee: nobody → pchang
Attached patch v1 (obsolete) — Splinter Review
Remove nsThread related stuff and change to PlatformThread::CurrentId() to get thread id
Attachment #8418607 - Flags: feedback?(cku)
Comment on attachment 8418607 [details] [diff] [review]
v1

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

good
Attachment #8418607 - Flags: feedback?(cku) → feedback+
Attachment #8418607 - Flags: review?(bjacob)
Comment on attachment 8418607 [details] [diff] [review]
v1

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

Very good catch regarding the unused DispatchToOwningThread. Thanks!

The other part of this thread is using a Chromium IPC PlatformThreadId instead of a nsCOMPtr<nsIThread>.

I need to ask :bsmedberg two questions:

 - Do we want to start using PlatformThreadId in general Gecko code like this, outside of specifically IPC-related code? According to
        http://dxr.mozilla.org/mozilla-central/search?q=%2Btype-ref%3APlatformThreadId
   so far PlatformThreadId is only used in IPC-facing code.

 - PlatformThreadId seems to be just a pid_t on posix. So a PlatformThreadId variable could outlive the thread that it's representing. Could there be a scenario where a thread finishes, and later a new thread is created with the same PlatformThreadId? Here we are only using this PlatformThreadId in an assertion that we're not accidentally using a GLContext on the wrong thread, so that's might be OK, but I would like to know if we're losing assert coverage.
Attachment #8418607 - Flags: feedback?(benjamin)
Our hybrid IPC/XPCOM threading mechanism is a mess that needs to be cleaned up. For now, if code is running on an XPCOM thread it should be using nsIThread APIs. If it is running on a chromium thread it should be using the Chromium APIs.

For assert coverage, I don't think the pid_t thing is an issue. In general I *think* the kernel/pthreads tries to avoid reusing thread IDs.
OK. The present code runs primarily from chromium IPC (especially in e10s / b2g scenarios) but can also run from a nsThread. Thanks also for clearing up my pid_t concern. So I suppose that the present patch is r+, then.
Attachment #8418607 - Flags: review?(bjacob)
Attachment #8418607 - Flags: review+
Attachment #8418607 - Flags: feedback?(benjamin)
Keywords: checkin-needed
Hi Peter,

could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
Attached patch v2(carry r+)Splinter Review
Update reviewer msg
Attachment #8419949 - Flags: review+
Attachment #8418607 - Attachment is obsolete: true
(In reply to Carsten Book [:Tomcat] from comment #8)
> Hi Peter,
> 
> could you provide a Try link. Suggestions for what to run if you haven't
> yet can be found here:
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Thanks for reminder.
https://tbpl.mozilla.org/?tree=Try&rev=22969c495b53
try server for debug build
https://tbpl.mozilla.org/?tree=Try&rev=acd1c247ddbe

Got positive result form opt/debug try run.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8c12405b970
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.