dom worker terminate() doesnt terminate proxied xhr requests

RESOLVED FIXED in Firefox 53

Status

()

--
critical
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: pdp, Assigned: amarchesini)

Tracking

({testcase})

unspecified
mozilla53
x86
All
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.18.1 (KHTML, like Gecko) Version/5.0.2 Safari/533.18.5
Build Identifier: Firefox/Xulrunner 3.6.10 1.9.2.10

XHR requests spawned/queued from a dom worker will still execute even when the worker is terminated by calling its terminate() method. This could lead to a crash on MacOS X with potential side-effects.

Reproducible: Always

Steps to Reproduce:
1. Go to http://websecurifydemo.appspot.com
2. Start test2
3. After 1-2 seconds stop test2
4. Repeat as much it is required
Actual Results:  
On windows the requests will still execute even when the worker is terminated... but the application is stable. On MacOS, the application may crash.

Expected Results:  
The terminate() method should free all resources associated with the worker.
(Reporter)

Comment 1

8 years ago
any progress on this bug?
The url in comment 0 no longer works. Does this still happen?
Status: UNCONFIRMED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INCOMPLETE

Comment 3

2 years ago
Created attachment 8818546 [details]
Testcase: Start loop of sync XHRs in worker and try to terminate it - watch as network requests in Developer Tools don't stop

The error is that the worker itself doesn't terminate while performing synchronous XMLHttpRequests. This is bad, because the only left usecase for sync XHRs is in webworkers.
They (the workers) should be terminateable under all circumstances.

Updated

2 years ago
Flags: needinfo?(bent.mozilla)

Comment 4

2 years ago
(In reply to cachius from comment #3)
In the testcase I use 127.0.0.2 to get a continuous loop of XHRs.
Also note that new XHRs are sent without considering the state of the previous one
and no exception occurs (which does in Chrome)
(or any reaction at all on the state of the previously sent XHRs)

This also happens when using xhr = new XMLHttpRequest(); inside the loop.


All recent versions are affected,
a workaround is to use asynchronous XHRs.

Updated

2 years ago
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(bent.mozilla) → needinfo?(bzbarsky)
Keywords: testcase
Resolution: INCOMPLETE → ---

Comment 5

2 years ago
bz, if you don't have any time to check this, could you NI? someone else from the DOM team, please.
Flags: needinfo?(bzbarsky) → needinfo?(amarchesini)
I guess we're waiting for the operation callback to fire so we can kill the script, but it may be a while before that happens....

Seems to me like any time we complete a syncloop we should consider triggering the operation callback.  Certainly any time we hit the mSyncLoopStack.IsEmpty() && mPendingEventQueueClearing case.

We should also consider explicitly throwing uncatchable exceptions from any place that spins a syncloop if a termination comes in while the syncloop spins...
Or more generally we could refuse attempts to post events back to the main thread (maybe except for certain whitelisted ones?) once a worker is supposed to have terminated and event queues cleared.  This would allow script to run for a bit longer on the worker, but without any observable effects.

Maybe we want both approaches.
Created attachment 8823076 [details] [diff] [review]
xhr.patch

The issue here is that we allow the creation of sync event loop at any state of the worker shutdown. This reminds me a similar issue with WorkerHolder so I propose a similar solution:

When a sync event loop needs to be created, a failStatus value must be passed. If the worker status is equal or greater than failStatus, the sync event loop is not created.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8823076 - Flags: review?(bkelly)
Comment on attachment 8823076 [details] [diff] [review]
xhr.patch

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

Overall looks reasonable.  I understand why Killing is used in most places, but I don't understand why you use Closing at a few call sites.

::: dom/base/WebSocket.cpp
@@ +336,5 @@
>      RefPtr<PrintErrorOnConsoleRunnable> runnable =
>        new PrintErrorOnConsoleRunnable(this, aBundleURI, aError, aFormatStrings,
>                                        aFormatStringsLen);
>      ErrorResult rv;
> +    runnable->Dispatch(Killing, rv);

Why does printing to the console have to happen even at Killing?

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +333,5 @@
>      MOZ_ASSERT(workerPrivate);
>  
>      RefPtr<InitializeRunnable> runnable =
>        new InitializeRunnable(workerPrivate, origin, principalInfo, aRv);
> +    runnable->Dispatch(Closing, aRv);

Why Closing instead of Terminating?

::: dom/network/ConnectionWorker.cpp
@@ +235,5 @@
>    RefPtr<ShutdownRunnable> runnable =
>      new ShutdownRunnable(mWorkerPrivate, this);
>  
>    ErrorResult rv;
> +  runnable->Dispatch(Killing /* This runnable _must_ be executed. */, rv);

nit: Can you use // comments above the line or trailing the line?  Here and elsewhere in the patch.

::: dom/workers/RuntimeService.cpp
@@ +622,5 @@
>      RefPtr<LogViolationDetailsRunnable> runnable =
>          new LogViolationDetailsRunnable(worker, fileName, lineNum);
>  
>      ErrorResult rv;
> +    runnable->Dispatch(Canceling, rv);

Why Canceling here?

::: dom/workers/ScriptLoader.cpp
@@ +2112,5 @@
>    NS_ASSERTION(!aLoadInfos.IsEmpty(), "Bad arguments!");
>  
> +  AutoSyncLoopHolder syncLoop(aWorkerPrivate, Terminating);
> +  nsCOMPtr<nsIEventTarget> syncLoopTarget = syncLoop.GetEventTarget();
> +  if (!syncLoopTarget) {

I wonder if we can ever get into Terminating before LoadAllScripts() is called.  Could we MOZ_DIAGNOSTIC_ASSERT(syncLoopTarget) here?  Maybe not worth it.

@@ +2113,5 @@
>  
> +  AutoSyncLoopHolder syncLoop(aWorkerPrivate, Terminating);
> +  nsCOMPtr<nsIEventTarget> syncLoopTarget = syncLoop.GetEventTarget();
> +  if (!syncLoopTarget) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Any reason not to use the DOM state error here?

::: dom/workers/WorkerNavigator.cpp
@@ +168,5 @@
>  
>    RefPtr<GetUserAgentRunnable> runnable =
>      new GetUserAgentRunnable(workerPrivate, aUserAgent);
>  
> +  runnable->Dispatch(Canceling, aRv);

Why Canceling here when you using Terminating most other places?

::: dom/workers/WorkerPrivate.h
@@ +1454,5 @@
>      memcpy(aPreferences, mPreferences, WORKERPREF_COUNT * sizeof(bool));
>    }
>  
>    already_AddRefed<nsIEventTarget>
> +  CreateNewSyncLoop(Status aFailStatus);

Please add a comment block documenting what aFailStatus is for and how to pick a value.

@@ +1529,5 @@
>    nsCOMPtr<nsIEventTarget> mTarget;
>    uint32_t mIndex;
>  
>  public:
> +  AutoSyncLoopHolder(WorkerPrivate* aWorkerPrivate, Status aFailStatus)

Please add a comment describing aFailStatus.  You could refer to the CreateNewSyncLoop() documentation.

::: dom/workers/WorkerRunnable.h
@@ +403,5 @@
>    // Dispatch the runnable to the main thread.  If dispatch to main thread
> +  // fails, or if the worker is in a state equal or greater of aFailStatus, an
> +  // error will be reported on aRv.  In that case the error MUST be propagated
> +  // out to script.
> +  void Dispatch(Status aFailStatus, ErrorResult& aRv);

Please add documentation describing how to pick aFailStatus.  For example, you probably want Terminating unless it should be an infallible runnable in which case you want Killing.
Attachment #8823076 - Flags: review?(bkelly) → review+
> Why does printing to the console have to happen even at Killing?

This sync loop cannot generate any other JS code running. Plus it's nice to have a console error reported for executed code, also if this happens after a worker.terminate().

> ::: dom/broadcastchannel/BroadcastChannel.cpp
> 
> Why Closing instead of Terminating?

Because few lines after we register a WorkerHolder with failStatus == Closing. And also in that case we abort the creation of the BroadcastChannel object.

All the other comments are applied.

Comment 11

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b786bf4dcdd
Sync event loops in workers should be created only if compatible with the worker shutdown status, r=bkelly

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b786bf4dcdd
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1331564
You need to log in before you can comment on or make changes to this bug.