Closed Bug 1272594 Opened 6 years ago Closed 6 years ago

Remove special cookie policy handling within InternalRequest

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 2 obsolete files)

No description provided.
The special code landed within Bug 1188822 [1]. We now have support for asyncOpen2() for stylesheets as well as imgLoader (See Bug 1206961 and Bug 1195173). I suppose we can remove that code now.

[1] https://hg.mozilla.org/mozilla-central/rev/74ab992b6952#l2.24
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Depends on: 1188822
Whiteboard: [domsecurity-active]
Ben, I am not entirely sure, but I suppose we can remove that special handling now that we support asyncOpen2() for images and styles also, right? Or am I missing something?
Attachment #8752102 - Flags: review?(bkelly)
Christoph, what code still uses AsyncOpen() in the tree?

If there is still a call site that uses AsyncOpen() and can be intercepted by service workers then we need to keep this code.  That particular test exercised images and stylesheets, but there could be other types of loads without test coverage.
Flags: needinfo?(ckerschb)
(In reply to Ben Kelly [:bkelly] from comment #3)
> Christoph, what code still uses AsyncOpen() in the tree?

We still have to convert:
* TYPE_OBJECT
* TYPE_DOCUMENT
* TYPE_WEBSOCKET

If you think we should rather wait till we have converted all the callsites to use AsyncOpen2(), then that's fine with me. We can keep this bug around and land once we are done converting the whole tree to rely on AsyncOpen2().

Feel free to clear the r?.
Flags: needinfo?(ckerschb) → needinfo?(bkelly)
Comment on attachment 8752102 [details] [diff] [review]
bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

TYPE_OBJECT and TYPE_WEBSOCKET are not intercepted by service workers.  We need to wait for TYPE_DOCUMENT to be converted to AsyncOpen2(), though.
Flags: needinfo?(bkelly)
Attachment #8752102 - Flags: review?(bkelly)
Depends on: 1182535
Ben, the old patch is outdated. I rebased the logic and I think we can only remove the switch statement as of now, right?

I think all the other parts need to be fixed within Bug 1189945.
Attachment #8752102 - Attachment is obsolete: true
Attachment #8823579 - Flags: review?(bkelly)
Comment on attachment 8823579 [details] [diff] [review]
bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

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

Sorry, I think this is removing the wrong code.  We need to modify MapChannelToRequestCredentials() not MapChannelToRequestMode().  We want to remove this bit:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.cpp#446-464
Attachment #8823579 - Flags: review?(bkelly) → review-
(In reply to Ben Kelly [:bkelly] from comment #7)
> Sorry, I think this is removing the wrong code.  We need to modify
> MapChannelToRequestCredentials() not MapChannelToRequestMode().  We want to
> remove this bit:

Facepalm - you are absolutely right. Never rebase patches in the middle of the night. Here we go!
Attachment #8823579 - Attachment is obsolete: true
Attachment #8823802 - Flags: review?(bkelly)
Comment on attachment 8823802 [details] [diff] [review]
bug_1272594_remove_special_cookie_policy_within_internalrequest.patch

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

Can you add a `MOZ_DIAGNOSTIC_ASSERT(loadInfo->GetSecurityMode() != nsILoadInfo::SEC_NORMAL)`? r=me with that change.
Attachment #8823802 - Flags: review?(bkelly) → review+
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f67c56b19396
Remove special cookie policy handling within InternalRequest (r=bkelly)
https://hg.mozilla.org/mozilla-central/rev/f67c56b19396
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.