Open Bug 767875 Opened 12 years ago Updated 2 years ago

xpcshell does not allow to create a worker from scripts loaded from a local URL without the protocol scheme.

Categories

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

defect

Tracking

()

People

(Reporter: sinker, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
We usually load a script with a local native path (an invalid URI), but WorkerPrivate always check for if it is valid URL.  So, we need a patch to make local native path as an URI automatically.
Try to treat an native file path as a local file URI.
Blocks: 705178
Component: DOM → DOM: Workers
QA Contact: general → workers
Attachment #636242 - Flags: review?(bent.mozilla)
ping for review
Jonas, what do you think about this behavior? Is this something we should allow?
This sounds like a good idea, but we should use a different constructor than just "Worker". Maybe something like "new LocalWorker"?
(In reply to Jonas Sicking (:sicking) from comment #5)
> This sounds like a good idea, but we should use a different constructor than
> just "Worker". Maybe something like "new LocalWorker"?

Agree!
Comment on attachment 636242 [details] [diff] [review]
handle local file path as script URIs of workers

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

Clearing review until a new patch shows up.
Attachment #636242 - Flags: review?(bent.mozilla)
Add LocalWorker handling local file pathes as script locations.
Attachment #636242 - Attachment is obsolete: true
Attachment #659274 - Flags: review?(bent.mozilla)
Comment on attachment 659274 [details] [diff] [review]
Handle local file pathes as script URIs of workers, v2

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

Thanks for being patient here. r=me with these changes:

::: dom/workers/RuntimeService.cpp
@@ +434,5 @@
>      if (isChrome && !chromeworker::InitClass(aCx, aObj, worker, true)) {
>        return false;
>      }
>  
> +    if (isChrome && !localworker::InitClass(aCx, aObj, worker, true)) {

Nit: Just combine this with the other isChrome check:

  if (isChrome && (!chromeWorker::InitClass(...) ||
                   !localWorker::InitClass(...)))

::: dom/workers/ScriptLoader.cpp
@@ +270,5 @@
>        nsCOMPtr<nsIURI> uri;
>        rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(uri),
>                                                       loadInfo.mURL, parentDoc,
>                                                       baseURI);
> +      if (NS_FAILED(rv) && mWorkerPrivate->IsLocalWorker()) {

This shouldn't be needed, right? You already might have prepended this in the worker constructor. I would expect this would make "file://file://foo.js"

::: dom/workers/Worker.cpp
@@ +400,5 @@
> +    return ConstructInternal(aCx,
> +                             aArgc,
> +                             aVp,
> +                             WorkerType::WT_CHROMEWORKER,
> +                             Class());

Nit: Please don't do one arg per line like this, here and below.

@@ +452,5 @@
> +/**
> + * Workers created from a local URL (without a scheme).
> + *
> + * It is avaiable only for chrome, but worker itself is not with
> + * chrome privilege.

Nit: "This type of worker may only be created from chrome but the resultant worker does not have chrome privileges."

@@ +545,5 @@
> +};
> +
> +// When this DOMJSClass is removed and it's the last consumer of
> +// sNativePropertyHooks then sNativePropertyHooks should be removed too.
> +DOMJSClass LocalWorker::sClass = {

Make sure everything that you copy/pasted here is still correct. There have been some JS engine changes since you wrote this the first time.

::: dom/workers/WorkerPrivate.h
@@ +164,5 @@
>  
> +  enum WorkerType {
> +    WT_WORKER = 0,
> +    WT_LOCALWORKER,
> +    WT_CHROMEWORKER

How about just 'Normal, Local, Chrome'? All caps make it look awful.

::: dom/workers/test/localWorker_worker.js
@@ +1,5 @@
> +/**
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +onmessage = function(event) {

Can you make sure that 'LocalWorker' and 'ChromeWorker' and something like 'ctypes' is not available here?
Attachment #659274 - Flags: review?(bent.mozilla) → review+
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: