Closed
Bug 761227
Opened 12 years ago
Closed 12 years ago
Support XMLHttpRequestParameters in workers
Categories
(Core :: DOM: Workers, defect, P1)
Tracking
()
People
(Reporter: philikon, Assigned: baku)
References
Details
(Keywords: feature, Whiteboard: [LOE:M][WebAPI:P3])
Attachments
(1 file, 6 obsolete files)
23.23 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → philipp
Reporter | ||
Comment 2•12 years ago
|
||
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?
Reporter | ||
Comment 3•12 years ago
|
||
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.
Reporter | ||
Comment 4•12 years ago
|
||
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
Reporter | ||
Comment 5•12 years ago
|
||
Nom'ing for basecamp. At least the PDF reader app needs this.
blocking-basecamp: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → -
Updated•12 years ago
|
blocking-basecamp: - → +
Reporter | ||
Comment 7•12 years ago
|
||
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.
Comment 8•12 years ago
|
||
I believe Chris +'d this because the PDF reader is in for v1.
Comment 9•12 years ago
|
||
Correct, we need to be able to read PDFs in v1 for the Email and Browser apps.
Reporter | ||
Comment 10•12 years ago
|
||
Ok. It's in my queue, but feel free to re-assign if this is needed sooner.
blocking-basecamp: + → ?
Priority: P1 → --
Whiteboard: [LOE:M]
Updated•12 years ago
|
blocking-basecamp: ? → +
Priority: -- → P1
Comment 11•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
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)
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.
Reporter | ||
Comment 14•12 years ago
|
||
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)
Assignee | ||
Comment 15•12 years ago
|
||
> 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.
Assignee | ||
Comment 16•12 years ago
|
||
Thank to your test I have found a bug in my patch. Uploading a new patch with the XHR_system test.
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #658052 -
Attachment is obsolete: true
Attachment #658052 -
Flags: review?(bent.mozilla)
Attachment #658831 -
Flags: review?(bent.mozilla)
Updated•12 years ago
|
Whiteboard: [LOE:M] → [LOE:M][WebAPI:P3]
Reporter | ||
Comment 18•12 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #16) > Thank to your test I have found a bug in my patch. Hooray tests! :)
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.
Assignee | ||
Comment 20•12 years ago
|
||
> 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.
Assignee | ||
Comment 22•12 years ago
|
||
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+
Assignee | ||
Comment 24•12 years ago
|
||
Attachment #661165 -
Attachment is obsolete: true
Attachment #661468 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 25•12 years ago
|
||
Can you please rebase this on top of bug 609748? It bitrots this quite a bit.
Keywords: checkin-needed
Assignee | ||
Comment 26•12 years ago
|
||
rebased
Attachment #661468 -
Attachment is obsolete: true
Attachment #661563 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 27•12 years ago
|
||
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
Comment 28•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #27) > https://tbpl.mozilla.org/?tree=Try&rev=ba1c84c4a83b Green on Try. https://hg.mozilla.org/integration/mozilla-inbound/rev/1909e0846920
Flags: in-testsuite+
Keywords: checkin-needed
Comment 29•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1909e0846920
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 30•12 years ago
|
||
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();
Reporter | ||
Comment 31•12 years ago
|
||
(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?
You need to log in
before you can comment on or make changes to this bug.
Description
•