Closed Bug 1148935 Opened 5 years ago Closed 4 years ago

Correctly reflect "worker" and "sharedworker" RequestContext values

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: Ehsan, Assigned: Ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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?
Flags: needinfo?(ehsan)
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.
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 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. )
Attachment #8585201 - Flags: review?(bugs) → review-
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?
Flags: needinfo?(ehsan)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bzbarsky)
Summary: 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 → Correctly reflect "worker" and "sharedworker" RequestContext values
Flags: needinfo?(bugs)
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.
Attachment #8623424 - Flags: review?(bugs) → review+
Depends on: 1175480
Attachment #8585201 - Attachment is obsolete: true
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
Flags: needinfo?(ehsan)
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...
Depends on: 1178339, 1163410
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/657293a5031a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Target Milestone: mozilla41 → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.