Test the Isolate the Image Cache per url bar domain (Tor 13749.2)

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: huseby, Assigned: huseby)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [tor-testing][OA-testing][domsecurity-backlog1][ETA 10/10])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Created attachment 8741292 [details] [diff] [review]
the Tor Browser patch (WIP)

Tor patch for testing isolation of the image cache.
(Assignee)

Updated

2 years ago
Summary: Isolate the Image Cache per url bar domain. → Test the Isolate the Image Cache per url bar domain.
Whiteboard: [tor][OA] → [tor][OA][domsecurity-backlog]
Here's a tracking link to the tor patch:
https://torpat.ch/13749
The "cache" test includes testing for images.
(Assignee)

Updated

2 years ago
Summary: Test the Isolate the Image Cache per url bar domain. → Test the Isolate the Image Cache per url bar domain (Tor 5742)
Whiteboard: [tor][OA][domsecurity-backlog] → [tor][OA-testing][domsecurity-backlog]
(Assignee)

Updated

2 years ago
Whiteboard: [tor][OA-testing][domsecurity-backlog] → [tor-testing][OA-testing][domsecurity-backlog]
(Assignee)

Updated

2 years ago
Priority: -- → P1

Updated

2 years ago
Depends on: 1289319
Priority: P1 → P3
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog] → [tor-testing][OA-testing][domsecurity-backlog1]

Updated

2 years ago
Priority: P3 → P1
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog1] → [tor-testing][OA-testing][domsecurity-backlog1][ETA 10/10]

Updated

2 years ago
Priority: P1 → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → huseby

Updated

2 years ago
See Also: → bug 962365

Updated

2 years ago
Blocks: 1299996
Summary: Test the Isolate the Image Cache per url bar domain (Tor 5742) → Test the Isolate the Image Cache per url bar domain (Tor 13749.2)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Created attachment 8790986 [details] [diff] [review]
Bug_1264572.patch

This patch adds a setup function to the testing framework so that local state can be modified before the tabs are created.

This test also confirms that the image cache isolation is working for first party domains.
Attachment #8741292 - Attachment is obsolete: true
Attachment #8790986 - Flags: review?(tihuang)
Attachment #8790986 - Flags: feedback?(arthuredelstein)

Comment 4

2 years ago
Comment on attachment 8790986 [details] [diff] [review]
Bug_1264572.patch

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

There are some problems need to be fixed here. Also add baku as a reviewer.

::: browser/components/originattributes/test/browser/browser_imageCacheIsolation.js
@@ +13,5 @@
> +let server = new HttpServer();
> +server.registerPathHandler('/image.png', imageHandler);
> +server.registerPathHandler('/file.html', fileHandler);
> +server.start(-1);
> +

The Http server should be stopped after the test is finished. Something like:

```js
registerCleanupFunction(() => {
  server.stop(() => {
    server = null;
  });
});
```

@@ +35,5 @@
> +  response.bodyOutputStream.write(body, body.length);
> +}
> +
> +function doSetup() {
> +  gHits = 0;

I think the image cache should be cleared here as well as you reset the gHits. Because the remaining cache would affect testing results.

::: browser/components/originattributes/test/browser/head.js
@@ +299,5 @@
>      this._add_task(function* (aMode) {
> +      if (aSetupFunc) {
> +        yield aSetupFunc();
> +      }
> +

The setup function should not be called here, it should be called right before tabs are created. The framework will test two cases of a given testing mode, including isolated tabs and non-isolated tabs. For example, it will test two tabs with the same first party domain and then different first party domains in the testing of first party isolation. So, if you put setup function here, only the first case will have a clean state, but the second one will not.
Attachment #8790986 - Flags: review?(tihuang) → review?(amarchesini)
Comment on attachment 8790986 [details] [diff] [review]
Bug_1264572.patch

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

Tim's comments are correct.
Attachment #8790986 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8791419 [details] [diff] [review]
WIP with clean up and image cache clearing code added

This is a WIP updated patch that includes clean up functions and image cache clearing.  The image cache clearing doesn't seem to work or that the image cache first party isolation isn't working.  Something is broken.  I'm trying to debug it.
(Assignee)

Comment 7

2 years ago
Do either of you have any idea why the image cache clearing doesn't seem to work?  The tests that fail are the second run of the non-isolated test where both image loads come from the cache when the first one should come from the web server.
Flags: needinfo?(tihuang)
Flags: needinfo?(amarchesini)

Comment 8

2 years ago
I have tested your test case with enabling logging of imgLib. It shows that the image cache clearing does work, but it seems the network request never comes out even if the image cache miss at the second run. I think the reason could be that the image has been cached by the Http cache, so there will be no real network request even if the image cache miss.
Flags: needinfo?(tihuang)
(Assignee)

Comment 9

2 years ago
Tim, how did you enable the imgLib logging?
Flags: needinfo?(amarchesini) → needinfo?(tihuang)
(Assignee)

Comment 10

2 years ago
Created attachment 8791676 [details] [diff] [review]
Bug_1264572.patch r+ :baku

r+ :baku

This updated patch adds image cache and network cache clearing between tests so we can be certain that the origin attribute isolation is working correctly.
Attachment #8790986 - Attachment is obsolete: true
Attachment #8791419 - Attachment is obsolete: true
Attachment #8790986 - Flags: feedback?(arthuredelstein)
Attachment #8791676 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Please reference here [1], and use the module "imgRequest".

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Gecko_Logging
Flags: needinfo?(tihuang)
(Assignee)

Comment 13

2 years ago
try looks green to me.  flagging for check in.
Keywords: checkin-needed

Comment 14

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32bb28f70092
Test the Isolate the Image Cache per url bar domain. r=baku
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/32bb28f70092
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

2 years ago
Depends on: 1304432
You need to log in before you can comment on or make changes to this bug.