1763240 - ReadableStream is defined on Sandbox global after accessing Response.body

Status: NEW

(Core :: DOM: Streams, defect, P2)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Essentially, in a content script test,

// This is the minimal example of the failure: By calling `Response.body` *before* first checking the ReadableStream property,
// we find that we’re somehow getting the *unwrapped* ReadableStream, which is deeply unexpected.
async function readable_stream_contentscript() {
  (await fetch(“http://example.com/index.html”)).body; // Causes below ReadableStream to be not the wrapped but the unwrapped. 

  browser.test.assertEq(
    “wrappedJSObject” in ReadableStream,
    true,
    “ReadableStream should be an Xray to the page version.”
  );

  browser.test.notifyPass(“finish”);
}

Not using the .body getter, or removing that line causes the test case to pass.

I have generated two Pernosco traces:

• One for the passing version of this where we don’t call .body
• One for the failing version, essentially the above.

This was discovered while re-arranging the test case from Bug 1757836 to be sure we tested and maintained the behaviour there.

I’ll attach a patch with my test case as well. I’ve tested some other APIs (.formData, .blob, and they don’t seem to show the same behaviour).

I'm opening this in DOM Streams for now, but it's likely not the right component for this to live in the end.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: WIP: Bug 1763240 - Test case for ReadableStream becoming unwrapped after using Response.body getter

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Zombie: Does this make any sense to you? Is this an expected outcome in running addons?

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

So I did a little more investigation here. What seems to happen is as follows (messily annotated on the failing pernosco trace linked to previously)

1. When we invoke the body getter on the Response returned by fetch, we create a new ReadableStream in the Sandbox global.
2. As a part of returning that ReadableStream, the binding code tries to create the reflector.
3. When wrapping that object, GetProtoObjectHandle is called, which in turn invokes dom::CreateInterfaceObjects; because defineOnGlobal is true, ReadableStream's constructor is installed on the Sandbox global.

So subsequently when we ask for the constructor, we get the unwrapped one installed on the Sandbox Global, not as you might expect the wrapped one from the window;

In the 'passing' version above, we instead go through the SandboxProxy::has path, and end up in XrayresolveOwnProperty, and so the binding is presumably wrapped in an Xray.

Peterv: It seems to me, maybe a getter returning a type should explicitly ask for a reflector where we don't define it on the global, to avoid this oddity?

(The gap in my knowledge is why other APIs like, formData and blob don't suffer similarly in my testing).

Comment 4

[Akoki]

Creating of ReadableStream are required for UserScript managers for implementing GM_xmlhttpRequest with responseType: "stream".

In this case the background script uses fetch and stream the data to user code to content/web script on the fly.
So, the user code can work with the partially downloaded data by reading body (ReadableStream object).

See more here: https://github.com/Tampermonkey/tampermonkey/issues/1278

Comment 5

[Tomislav Jovanovic :zombie]

I don't think I understand the underlying bindings/xray wrappers code to be of much help here, beyond the workaround that Luca already suggested: using window.wrappedJSObject.ReadableStream for the constructor.

Other than Peter, Kris do you maybe understand what should happen here?

Comment 6

[Kris Maglione [:kmag]]

We should perhaps avoid defining the interface objects at least in non-system-principal Sandboxes in this case. It seems unfortunate to have the behavior arbitrarily change after using certain APIs.

Alternatively, we could define it eagerly instead. I'm not sure whether that's better or worse. There are other APIs that are eagerly defined and shadow the X-ray wrapped content versions, and they seem to cause people who don't understand wrappers a different set of problems.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

This sounds like a policy decision around WebExtension behaviours; who would make that call?

Comment 8

[Tomislav Jovanovic :zombie]

If my vague understanding is right, if we don't define those interface objects on the Sandbox global (but the sandbox inherits them from the WindowGlobal, which is in its prototype chain), then there shouldn't be much visible difference for extensions, right?

Comment 9

[Akoki]

I find the suggestion to use window.wrappedJSObject.ReadableStream questionable.

As I understand, window.wrappedJSObject is used to get objects from the web page context. In fact the main case of using of it is to get access to some data defined by site's JavaScript code.

Why I should use ReadableStream from the web page context in the content script?
It does not makes any sense.

It's not some user defined object, that I should to get from the web page. It's the basic JS class. It should be accessible in the content script. And it's accessible. But currently it works with bugs.

It worked fine before. Now it works with bugs.

Comment 10

[Akoki]

Is there any progress in fixing it?

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

Sorry, no progress at the moment.

Comment 12

[marcos.pierandrei]

I have the same problem reported. In chrome and previous versions to firefox 100, they work fine, but new firefox versions give the same as you reported.

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #7)

This sounds like a policy decision around WebExtension behaviours; who would make that call?

Hey Will,

Wondering if you could help answer this question :)

Comment 14

[Shane Caraveo (:mixedpuppy)]

Matthew, what I am not clear about is what the solution is, and whether/how this affects addons. This bug has a lot of "we could do this, or we could do that" commentary. That makes a decision here a bit difficult.

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

Hey Shane,

The problem here being we've sort of got two parallel bug tracks running, which have some cross-impact on each other.

This bug specifically is trying to answer the question: "Is it OK if the wrapped/unwrapped status of a constructor is visibly able to change depending on what code runs earlier in the extension", and if no, what's the best final state: Should the ReadableStream constructor be an X-ray -always- or should it be an X-ray -never-, and instead be an interface on the sandbox.

If the answer is -never-, I think then if we fix this one, Bug 1757836 gets fixed as well, as it's a concrete example of the complexity that occurs because of web-extensions getting the xray to ReadableStream -- on that bug, you can see the complexity of the workarounds required to handle that.

However, I'm unclear of the implications of all this, and how this ties into a general policy for DOM APIs exposed in web-extension sandboxes.

Comment 16

[Rob Wu [:robwu]]

I'm not sure if the observed behavior is well understood here, so I'll elaborate on my interpretation of the observed behavior.

In the test case, essentially the two observed behaviors are:

1. Access to ReadableStream -> XrayWrapped version of window.ReadableStream.
2. (await fetch(...)).body -> non-XrayWrapped version of ReadableStream associated with the Sandbox global.

The first observation is because the Sandbox doesn't have a ReadableStream global by default, so the property is looked up from the prototype chain of the sandbox, i.e. window. Tom already mentioned this in comment 8.

The second observation is because the fetch method in an extension content script is associated with the Sandbox global, i.e. due to "fetch" being part of wantGlobalProperties (source code: Sandbox for content scripts and Sandbox for user scripts). Consequently, when (await fetch(...)).body is read, ReadableStream is lazily exported to the Sandbox. This is not a problem in Manifest V3, because I removed the Sandbox-specific fetch in https://hg.mozilla.org/mozilla-central/rev/36906ca5d8d7.

If we briefly ignore the extension angle for now, the summarize of the issue is that the use of fetch results in exports of previously-unseen globals in the Sandbox global scope. Despite the report claiming the opposite, this issue is not specific to ReadableStream, and also happens to .formData and .blob. The test cases that tried to prove the opposite were flawed, because they just did .formData, .blob, etc. While .body is indeed a getter that returns the ReadableStream instance, the .formData, .blob, etc. properties are not getters but methods.

Here is a minimal test case to show the similar issue with .blob:

{
let sand = Cu.Sandbox(null, {wantGlobalProperties:["fetch"]});
Cu.evalInSandbox(`
(async() => {
  let before = typeof Blob;
  await ((await fetch("https://cors-anywhere.herokuapp.com")).blob());
  let after = typeof Blob;
  return [before, after];
})()
`, sand).then(console.log)
// Array [ "undefined", "function" ]
}

There are two ways to get consistent behavior. Either by not defining the interface object on the sandbox global when not part of the wantGlobalProperties array (even if fetch instantiates a ReadableStream), or to unconditionally define them. The latter is done for Request, Response and Headers at https://searchfox.org/mozilla-central/rev/5bcbe6ae54ee5b152f6cef29dc5627ec7c0e1f1e/js/xpconnect/src/Sandbox.cpp#349-352 (but not when ReadableStream was added in bug 1746653).
Kris already mentioned these two options in comment 6, and remarked:

Alternatively, we could define it eagerly instead. I'm not sure whether that's better or worse. There are other APIs that are eagerly defined and shadow the X-ray wrapped content versions, and they seem to cause people who don't understand wrappers a different set of problems.

I share the concern about misunderstandings: the mix of wrapped DOM APIs from the page and content script sandbox is already causing confusion among extension developers. We have many bug reports caused by the current implementation (see bug 1208775 and its related bugs).

But the inconsistency due to the lazy initialization as observed in this bug is not great either... It would be ideal if DOM APIs could behave consistently regardless of the realm they're in. That was the original intent behind XrayWrappers, that JS code using DOM APIs get the native behavior, but clearly that's not the case as observed in bug 1757836 (i.e. the ReadableStream class through a XrayWrapper is observably broken).

In this bug specifically, we should look into trying to achieve consistent behavior (that accessing random members of fetch does not result in surprising new globals in the Sandbox).
In bug 1757836 we can focus on the desired way of exposing ReadableStream to content scripts in extensions.

Comment 17

[Jens Stutte [:jstutte]]

:saschanaz, can you briefly verify the state of this bug? I assume the ni? to :peterv was outdated.

Comment 18

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: WIP: Bug 1763240 - Test case for ReadableStream becoming unwrapped after using Response.body getter

Comment 19

[Kagami Rosylight [:saschanaz] (they/them)]

Yes the problem still exists, basically as what Rob summarized in comment #16.