1148935 - Correctly reflect "worker" and "sharedworker" RequestContext values

Status: RESOLVED FIXED

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

Comment 1

[(no longer active)]

Attachment: Split nsIContentPolicy::TYPE_SCRIPT into TYPE_SCRIPT, TYPE_WORKER, TYPE_SHAREDWORKER and TYPE_SERVICEWORKER so that Request.context can reflect the "worker", "sharedworker" and "serviceworker" values correctly

Comment 2

[Boris Zbarsky [:bzbarsky]]

What's our compat story for extensions with these changes?  We're making changes that look API and ABI compatible, but will break existing content policy implementations in extensions, no?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

The addons compat story has been discussed in other relevant bugs.
All these nsIContentPolicy changes will break tons of addons, so need to be rather careful. All the changes should go in at once, after notifying addon authors about the changes.
(Or some non-breaking way to deal with this stuff need to be found.)
In principle it would be nice if nsIContentPolicy load types would map to some spec, but perhaps
there are too many addons to update that we can't really do it this way.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Looking addons' nsIContentPolicy usage some more, I think this approach might not be doable after all. Just too many addons would be broken, way too many.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8585201 [details] [diff] [review]
Split nsIContentPolicy::TYPE_SCRIPT into TYPE_SCRIPT, TYPE_WORKER, TYPE_SHAREDWORKER and TYPE_SERVICEWORKER so that Request.context can reflect the "worker", "sharedworker" and "serviceworker" values correctly

So, this is r- for all the similar nsIContentPolicy changes, until it is figured out what can be done and what should be done.
(I know I gave r+ to some other changes, but I didn't realize ServiceWorker would need to change nsIContentPolicy so much. )

Comment 6

[(no longer active)]

I thought about this a bit more, and if we cannot change the nsContentPolicyTypes, the only way around it would be to store the RequestContext separately alongside the nsContentPolicyTypes and use the RequestContext in our code where more granular information about the load is needed (such as obviously when reflecting Request.context!).

How does that sound?  I think that sucks on some level because now we'd have two types that contain very similar but subtly different information, but perhaps it's not too had if we cannot change the existing context types?

Also, a question on the mechanics of the change.  RequestContext is an enum class generated by the WebIDL codegen.  Is that OK to be used in Necko (for example in LoadInfo) or do I need to make up my own enum that basically maps directly to RequestContext inside Necko?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I agree that sucks.  If it were not for the fact that the failure mode of an addon not being updated to the new setup is security holes, basically (e.g. noscript allowing script to run), I would say we should just change the existing nsContentPolicyTypes...

Necko is already including various binding headers.  See <http://mxr.mozilla.org/mozilla-central/search?string=binding.h&find=netwerk>.  So just using the binding enum directly in necko seems like it would be fine to me.

Comment 8

[(no longer active)]

Attachment: Correctly reflect worker and sharedworker RequestContext values

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8623424 [details] [diff] [review]
Correctly reflect worker and sharedworker RequestContext values

+    switch (aWorkerType) {
+    default:
+      MOZ_ASSERT_UNREACHABLE("Invalid worker type");
+    case WorkerTypeDedicated:
+      return nsIContentPolicy::TYPE_INTERNAL_WORKER;
+    case WorkerTypeShared:
+      return nsIContentPolicy::TYPE_INTERNAL_SHARED_WORKER;
+    case WorkerTypeService:
+      return nsIContentPolicy::TYPE_SCRIPT;
+    }
+  }
Please put default: to last.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/439e1f841ea2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=10904442&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1eac8fb874

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b09594541f3

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out again, seems this and the other landing caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10918636&repo=mozilla-inbound

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c66ab06d38

Comment 15

[(no longer active)]

Bug 1178339 and bug 1163410 should address the remaining issues in this test on Android.  I'm going to reland this patch while they are waiting for review...

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07bf31cca660

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for Werror bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=11224618&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/da4e8214484b

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/657293a5031a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/657293a5031a