Closed Bug 1116697 Opened 5 years ago Closed 5 years ago

nsScriptError::InitWithWindowID doesn't work off the main thread

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Details

Attachments

(2 files)

nsScriptError::InitWithWindowID accesses an unprotected hashtable of nsGlobalWindow from any thread on which it's called. nsScriptError is supposed to be threadsafe, so I think we should fix this somehow...
This patch adds assertions around the nsGlobalWindow::sWindowsById hashtable so that we don't accidentally do this again.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #8542817 - Flags: review?(mrbkap)
This part fixes nsScriptError to work on any thread... Kinda.

I think we should do this for now since it has the least risk of breaking things. We could go further though and just make this class entirely main-thread only...
Attachment #8542818 - Flags: review?(mrbkap)
Attachment #8542817 - Flags: review?(mrbkap) → review+
Comment on attachment 8542818 [details] [diff] [review]
Part 2: Fix nsScriptError to only do DOM stuff on the main thread, v1

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

A couple of small questions, this looks good otherwise.

::: js/xpconnect/src/nsScriptError.cpp
@@ +43,5 @@
> +{
> +    MOZ_ASSERT(NS_IsMainThread());
> +    MOZ_ASSERT(!mInitializedOnMainThread);
> +
> +    mInitializedOnMainThread = true;

Should this happen after we've set the members, so that mInitializedOnMainThread implies that mOuterWindowID is valid?

@@ +243,5 @@
>  
>  NS_IMETHODIMP
>  nsScriptError::GetOuterWindowID(uint64_t *aOuterWindowID)
>  {
> +    NS_WARN_IF_FALSE(NS_IsMainThread() || mInitializedOnMainThread,

Can this happen? Should we assert instead of warn here? My feeling these days is that warnings are basically ignored.
Attachment #8542818 - Flags: review?(mrbkap) → review+
Comment on attachment 8542818 [details] [diff] [review]
Part 2: Fix nsScriptError to only do DOM stuff on the main thread, v1

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

::: js/xpconnect/src/nsScriptError.cpp
@@ +43,5 @@
> +{
> +    MOZ_ASSERT(NS_IsMainThread());
> +    MOZ_ASSERT(!mInitializedOnMainThread);
> +
> +    mInitializedOnMainThread = true;

I guess... Right now it's a wash because there are no early returns, but I guess I was thinking that if there were any added they would have to make sure to either leave the defaults or set things correctly. I can move it, but then we risk early returns not setting this variable.

@@ +243,5 @@
>  
>  NS_IMETHODIMP
>  nsScriptError::GetOuterWindowID(uint64_t *aOuterWindowID)
>  {
> +    NS_WARN_IF_FALSE(NS_IsMainThread() || mInitializedOnMainThread,

It's part of an XPCOM interface so I don't think we have much choice. I could assert but that might break addons or something not tested on our infra at the moment.
Attachment #8542818 - Flags: feedback?(mrbkap)
Comment on attachment 8542818 [details] [diff] [review]
Part 2: Fix nsScriptError to only do DOM stuff on the main thread, v1

We talked about this in person.
Attachment #8542818 - Flags: feedback?(mrbkap) → feedback+
https://hg.mozilla.org/mozilla-central/rev/7070c9621457
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.