Closed Bug 761227 Opened 12 years ago Closed 12 years ago

Support XMLHttpRequestParameters in workers

Categories

(Core :: DOM: Workers, defect, P1)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: philikon, Assigned: baku)

References

Details

(Keywords: feature, Whiteboard: [LOE:M][WebAPI:P3])

Attachments

(1 file, 6 obsolete files)

Bug 692677 added an optional parameter for the XMLHttpRequest constructor, but it's ignored in workers.
Ugh, we need to stop taking patches for XHR that don't include the worker bits.
Assignee: nobody → philipp
Attached patch WIP 1 (tests only) (obsolete) — Splinter Review
Here's a WIP in which I've refactored soem of the tests to be usable within workers, too. I'm unsure how to proceed with the implementation since the Proxy and mainthread nsXMLHttpRequest objects are instantiated when xhr.open() is called. So presumably we want to change that, or we do the permission check in the XMLHttpRequest constructor but defer setting mIsAnon and mIsSystem to when the Proxy and the mainthread XHR objects are initialized.

bent, any thoughts?
Talked to bent offline. Summary:

* We evaluate the permission situation at the time the worker is created.
* XMLHttpRequest::Constructor converts aParams into an XMLHttpRequestParameters object.
* The XMLHttpRequestParameters gets passed to Proxy which passes it to nsIXMLHttpRequest::Init.
* nsXMLHttpRequest::Construct will unify reading XMLHttpRequestParameters.
Attached patch WIP 2 (obsolete) — Splinter Review
Rebased after bug 765468 landed, also started peeking into the worker code, but random reviews and PTOs caused distractions.
Attachment #632487 - Attachment is obsolete: true
Nom'ing for basecamp. At least the PDF reader app needs this.
blocking-basecamp: --- → ?
blocking-basecamp: ? → -
blocking-basecamp: - → +
How are we doing here?
Priority: -- → P1
I had put it on the back burner because we minused it for blocking in the triage. But I see clee plused it again (why?) so I guess I'll move it up in the queue again.
I believe Chris +'d this because the PDF reader is in for v1.
Correct, we need to be able to read PDFs in v1 for the Email and Browser apps.
Ok. It's in my queue, but feel free to re-assign if this is needed sooner.
blocking-basecamp: + → ?
Priority: P1 → --
Whiteboard: [LOE:M]
blocking-basecamp: ? → +
Priority: -- → P1
Andrea, can you please take the work that Philipp started here and finish it up? Philipp is swamped with other stuff and would love help driving this to completion.
Assignee: philipp → amarchesini
Attached patch patch 1 (obsolete) — Splinter Review
This patch is almost completed. I'm testing it. /me asking for a feedback.
Attachment #636559 - Attachment is obsolete: true
Attachment #658052 - Flags: review?(philipp)
Attachment #658052 - Flags: review?(bent.mozilla)
Depends on: 788149
Comment on attachment 658052 [details] [diff] [review]
patch 1

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

::: dom/workers/XMLHttpRequest.cpp
@@ +874,5 @@
>      return NS_OK;
>    }
>  };
>  
> +class ParamsCheckRunnable : public nsRunnable

This needs to reuse the WorkerThreadProxySyncRunnable I think.
Comment on attachment 658052 [details] [diff] [review]
patch 1

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

I can't really say anything about the worker code, but I would like to point out that my WIP patches also refactored the test_XHR_system.{js|html} tests. Any reason you didn't include them?
Attachment #658052 - Flags: review?(philipp)
> This needs to reuse the WorkerThreadProxySyncRunnable I think.

I cannot because at that time, I don't have the Proxy.
The proxy is called when 'open()' is called.
Thank to your test I have found a bug in my patch. Uploading a new patch with the XHR_system test.
Attached patch patch 2 (obsolete) — Splinter Review
Attachment #658052 - Attachment is obsolete: true
Attachment #658052 - Flags: review?(bent.mozilla)
Attachment #658831 - Flags: review?(bent.mozilla)
Whiteboard: [LOE:M] → [LOE:M][WebAPI:P3]
(In reply to Andrea Marchesini (:baku) from comment #16)
> Thank to your test I have found a bug in my patch.

Hooray tests! :)
Keywords: feature
Comment on attachment 658831 [details] [diff] [review]
patch 2

Hm. So, as was mentioned in comment 3, I think we should be doing all the permissions work up front when the worker is created rather than each time you manipulate an XHR object. Is this not possible for some reason (like, can the permission change during the lifetime of the worker)?

I'd really prefer we go that route if at all possible.
> Hm. So, as was mentioned in comment 3, I think we should be doing all the
> permissions work up front when the worker is created rather than each time
> you manipulate an XHR object. Is this not possible for some reason (like,
> can the permission change during the lifetime of the worker)?

I think yes, the permission can change during the lifetime of the worker.
This is something I test with a mochitest (test_xhr_parameters.*). If we implement this permission check only when the worker is created, the behaviours of XHR on main thread and on workers will be different.
(In reply to Andrea Marchesini (:baku) from comment #20)
> I think yes, the permission can change during the lifetime of the worker.
> This is something I test with a mochitest (test_xhr_parameters.*).

Even if you do the sync check you're still not guaranteed to have the correct permission... It's a race.

Anyway, I just talked to sicking and we both agree that we should not handle this case. Realistically speaking we're going to have to take further steps (like killing the app) if we want this to work reliably. It's not likely that we're going to provide the user with a knob to twist for this permission anyway.

So let's grab the permission when we create the worker. That should avoid all the sync headaches.
Attached patch patch 3 (obsolete) — Splinter Review
The check is done when the WorkerPrivate is created.
Attachment #658831 - Attachment is obsolete: true
Attachment #658831 - Flags: review?(bent.mozilla)
Attachment #661165 - Flags: review?(bent.mozilla)
Comment on attachment 661165 [details] [diff] [review]
patch 3

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

Thanks! This looks great.

::: content/base/src/nsXMLHttpRequest.h
@@ +202,5 @@
>    // Initialize XMLHttpRequestParameter object.
>    nsresult InitParameters(JSContext* aCx, const jsval* aParams);
>    void InitParameters(bool aAnon, bool aSystem);
>  
> +  void SetParameters(bool aAnon, bool aSystem) {

Nit: { on its own line

::: dom/workers/WorkerPrivate.cpp
@@ +2998,5 @@
>    return result;
>  }
>  
>  bool
> +WorkerPrivate::CheckXHRParamsAllowed(nsPIDOMWindow* aWindow)

Please assert that this is only called on the main thread.

@@ +3000,5 @@
>  
>  bool
> +WorkerPrivate::CheckXHRParamsAllowed(nsPIDOMWindow* aWindow)
> +{
> +  if (!aWindow || !aWindow->GetDocShell())

Instead of checking just assert non-null window. Then check for just the null docshell.

Also, please brace all your if statements here.

@@ +3007,5 @@
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(aWindow->GetExtantDocument());
> +  if (!doc)
> +    return false;
> +
> +  nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();

Can you just put |doc->NodePrincipal()| in the call to TestPermissionFromPrincipal and avoid this stack comptr?

::: dom/workers/WorkerPrivate.h
@@ +680,5 @@
>    bool
>    DisableMemoryReporter();
>  
> +  bool
> +  XHRParamsAllowed()

Nit: const

::: dom/workers/XMLHttpRequest.h
@@ +264,5 @@
>      mStateData.mResponseText.SetIsVoid(true);
>      mStateData.mResponse = JSVAL_NULL;
>    }
>  
>    bool MozAnon() {

Nit: I know this was here before, but can you make this (and MozSystem) const and put the { on its own line?
Attachment #661165 - Flags: review?(bent.mozilla) → review+
Attached patch patch 4 (obsolete) — Splinter Review
Attachment #661165 - Attachment is obsolete: true
Attachment #661468 - Flags: review+
Keywords: checkin-needed
Can you please rebase this on top of bug 609748? It bitrots this quite a bit.
Keywords: checkin-needed
Attached patch patch 4bSplinter Review
rebased
Attachment #661468 - Attachment is obsolete: true
Attachment #661563 - Flags: review+
Keywords: checkin-needed
Pushed to Try since I don't see any links here. I'll land it if it's green.
https://tbpl.mozilla.org/?tree=Try&rev=ba1c84c4a83b
https://hg.mozilla.org/mozilla-central/rev/1909e0846920
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 661563 [details] [diff] [review]
patch 4b

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

::: dom/workers/WorkerPrivate.cpp
@@ +3025,5 @@
> +  if (!aWindow->GetDocShell()) {
> +    return false;
> +  }
> +
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(aWindow->GetExtantDocument());

nsCOMPtr<nsIDocument> doc = aWindow->GetExtantDoc();
(In reply to :Ms2ger from comment #30)
> > +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(aWindow->GetExtantDocument());
> 
> nsCOMPtr<nsIDocument> doc = aWindow->GetExtantDoc();

This bug is RESO/FIXED... file a follow-up?
No longer depends on: 803225
You need to log in before you can comment on or make changes to this bug.