realm / realm-js

Realm is a mobile database: an alternative to SQLite & key-value stores
https://realm.io
Apache License 2.0
5.8k stars 576 forks source link

calling stream.return() on chrome 124 crashes browser #6634

Closed sharifmacky closed 5 months ago

sharifmacky commented 7 months ago

Calling return() on a change stream using the Websdk on Chrome Android 124 causes the browser to crash, showing "Aw, Snap! Something went wrong..." Works fine on Desktop. 123

Stepping through the sdk source it seems like on desktop Chrome 123, i.return() is a wrapper which then calls cancel() on the underlying stream reader. On mobile 124, i.return is a native fn.

return(value) {
        iterator.then((i) => i.return ? i.return(value) : void 0);  // <--- i.return is native on 124 
        return originalReturn.call(stream, value); // this is fine
}

On mob 124, it calls return() on the native fn and then steps into the watch() impl. After that the browser crashes.

Specifically I noticed that in the asyncIteratorFromResponseBody function, requests on a mobile browser 124 seem to have a Symbol.asyncIterator in the body and so don't get wrapped as do the requests from a desktop browser 123. If I comment the 'else if' block out, everything "seems" to work. Although I'm sure there's a better fix

function asyncIteratorFromResponseBody(body) {
  if (typeof body !== "object" || body === null) {
    throw new Error("Expected a non-null object");
  } else if (Symbol.asyncIterator in body) {   // <---------- this kills chrome on 124
    return body;                              // commenting this if block out "fixes" it
  } else if ("getReader" in body) {
    const stream = body;
    return {
      [Symbol.asyncIterator]() {
        const reader = stream.getReader();
        return {
          next() {
            return reader.read();
          },
          async return() {
            await reader.cancel();
            return { done: true, value: null };
          }
        };
      }
    };
  } else {
    throw new Error("Expected an AsyncIterable or a ReadableStream");
  }
}

I've yet to test on IOS.

Mobile Chrome: 124.0.6367.54 Android 13; KB2003 Build/RKQ1.211119.001

Desktop Chrome: 123.0.6312.106 Verified on 124.0.6367.61 Windows 11 Pro, OS build 22631.3447, Feature Experience Pack 1000.22688.1000.0

sync-by-unito[bot] commented 7 months ago

➤ PM Bot commented:

Jira ticket: RJS-2809

kneth commented 7 months ago

Thank you for reporting.

Symbol.asyncIterator should be supported by Chrome on Android. We need to investigate further.

Is it possible for you to upgrade your Desktop Chrome to 124 to see if it works or doesn't work?

sharifmacky commented 7 months ago

Yes it seems like a chrome 124 issue. On chrome desktop the same behaviour is now happening where Symbol.asyncIterator is in the response body and the browser crashes.

Dv-Andrew commented 7 months ago

Hi. We have a similar issue in our React web app. After upgrading to the latest version of Chrome (or Edge, and other Chromium-based browsers), our users encountered a constantly occurring "Aw, Snap!"-like error.

During the investigation, we found that the problem only occurs on newer versions of Chromium (v124) and is related to the use of change streams. All previous Chromium versions are working fine for us (v123 included). Firefox and Safari working fine too. Also, it looks like the problem is not related to the OS, Сhromium crashes on Windows and Mac OS (but we have not tested any Linux distros yet).

Tests show that just using the asyncIterator doesn't cause any problems, but for some reason when we use an asyncGenerator (AsyncGenerator<Realm.Services.MongoDB.ChangeEvent>), a memory leak occurs, which leads to a crash of the browser tab. At the moment, this is all that we have been able to find, but I will be glad to share any info if we find anything else.

sharifmacky commented 7 months ago

https://github.com/realm/realm-js/blob/6269081d4e58c116ea7800bdd18c59dc0ffd9c80/packages/realm-web/src/Fetcher.ts#L42-L43

andy-dextrous commented 7 months ago

Hi. We have a similar issue in our React web app. After upgrading to the latest version of Chrome (or Edge, and other Chromium-based browsers), our users encountered a constantly occurring "Aw, Snap!"-like error.

During the investigation, we found that the problem only occurs on newer versions of Chromium (v124) and is related to the use of change streams. All previous Chromium versions are working fine for us (v123 included). Firefox and Safari working fine too. Also, it looks like the problem is not related to the OS, Сhromium crashes on Windows and Mac OS (but we have not tested any Linux distros yet).

Tests show that just using the asyncIterator doesn't cause any problems, but for some reason when we use an asyncGenerator (AsyncGenerator<Realm.Services.MongoDB.ChangeEvent>), a memory leak occurs, which leads to a crash of the browser tab. At the moment, this is all that we have been able to find, but I will be glad to share any info if we find anything else.

Exactly the same happening for me and my users re Change Streams.

kneth commented 7 months ago

@Dv-Andrew Thank for all the details. @andy-dextrous Thank for confirming.

I will bring it up with the team to see what we can do but it might take a while.

piesuke commented 6 months ago

@kneth Were there any updates? I am having a lot of difficulty.

kneth commented 6 months ago

@piesuke Unfortunately we haven't any updates to share yet.

mevilbhojani commented 6 months ago

Our team also face similar issue .

kneth commented 6 months ago

@mevilbhojani @piesuke @Dv-Andrew @andy-dextrous @sharifmacky

I am using Chrome 124.0.6367.208 (Official Build) (arm64) on MacOS 14.5, and so far I haven't been able to reproduce it.

index.html:

<html>
  <head>
    <script src="https://unpkg.com/realm-web/dist/bundle.iife.js"></script>
    <script src="main.js"></script>
  </head>

  <body>
    <h1>RJS-2809</h1>
    <button type="button" onclick="main()">Start</button>
  </body>
</html>

main.js:

async function main() {
  const APP_ID = "<my secret>;
  const DATA_SOURCE_NAME = "<my secret>";
  const DATABASE_NAME = "<my secret>";
  const COLLECTION_NAME = "person";

  const app = new Realm.App(APP_ID);
  const creds = Realm.Credentials.anonymous();
  const user = await app.logIn(creds);
  console.log("User logged in: ", user.id);

  const mongo = app.currentUser.mongoClient(DATA_SOURCE_NAME);
  const collection = mongo.db(DATABASE_NAME).collection(COLLECTION_NAME);

  for await (const change of collection.watch()) {
    console.log("Change: ", change);
  }
}

I am pumping data to my database with the following script:

const dbUser = encodeURIComponent("<my secret>");
const dbPassword = encodeURIComponent("<my secret>");

const { MongoClient, ServerApiVersion } = require('mongodb');
const uri = `mongodb+srv://${dbUser}:${dbPassword}@<my secret>/?retryWrites=true&w=majority&appName=<my secret>`;

const client = new MongoClient(uri, {
  serverApi: {
    version: ServerApiVersion.v1,
    strict: true,
    deprecationErrors: true,
  }
});

async function sleep(ms) {
  return new Promise((resolve, reject) => {
    setTimeout(() => resolve(), ms);
  });
}

async function run() {
  try {
    await client.connect();
    await client.db("admin").command({ ping: 1 });
    console.log("Pinged your deployment. You successfully connected to MongoDB!");

    const collection = client.db("<my secret>").collection("person");

    let i = 0;
    while (true) {
      await collection.insertOne({
        name: `Jens-${i}`
      });
      console.log(`Added ${i}`);
      i++;
      await sleep(2000);
    }
  } finally {
    await client.close();
  }
}
run().catch(console.dir);

I am curious to understand how my test differs from your apps. Also, my documents are very small (one property, short string as value).

DeltaSPb commented 6 months ago

Same problem in our web app, but in my case I get a Chrome page crash (my version is 124.0.6367.208) when I try to close the connection by calling the return method on the async generator:

  await generator.return(null); // "Aw, Snap!" message after this call

Maybe this will help someone

andy-dextrous commented 6 months ago

@kneth Try adding another button to your html that returns the change stream. It's specifically the .return() function that causes the crash for me. I am using Next JS and Gatsby for two different sites and the return function crashes both. They are totally fine at processing changes until I call return.

kneth commented 6 months ago

Updated main.js to:

let watcher;

async function main() {
  const APP_ID = "<my secret>";
  const DATA_SOURCE_NAME = "<my secret>";
  const DATABASE_NAME = "<my secret>";
  const COLLECTION_NAME = "person";

  const app = new Realm.App(APP_ID);
  const creds = Realm.Credentials.anonymous();
  const user = await app.logIn(creds);
  console.log("User logged in: ", user.id);

  const mongo = app.currentUser.mongoClient(DATA_SOURCE_NAME);
  const collection = mongo.db(DATABASE_NAME).collection(COLLECTION_NAME);

  const list = document.getElementById("list");
  watcher = collection.watch();
  for await (const change of watcher) {
    console.log("Change: ", change);
  }
}

async function stop() {
  console.log("stopping watcher");
  await watcher.return();
  console.log("done");
}

and index.html to:

<html>
  <head>
    <script src="https://unpkg.com/realm-web/dist/bundle.iife.js"></script>
    <script src="main.js"></script>
  </head>

  <body>
    <h1>RJS-2809</h1>
    <button type="button" onclick="main()">Start</button>
    <button type="button" onclick="stop()">Stop</button>
  </body>
</html>

With Chrome 125 (today's update from the IT department), I am not able to reproduce it.

Screenshot 2024-05-16 at 17 58 43

@andy-dextrous If you can provide a simple reproduction case, it will be appreciated!

DeltaSPb commented 6 months ago

@kneth

I used your code and the problem is still reproducing. Chrome version - 125.0.6422.61 (Official Build) (x86_64)

Complete structure of my test directory **package.json:** ``` { "name": "change-stream", "version": "1.0.0", "main": "index.js", "license": "MIT", "scripts": { "start": "parcel index.html" }, "devDependencies": { "babel-eslint": "^10.1.0", "eslint": "^9.2.0", "parcel": "^2.12.0" } } ``` **index.html:** ```

Change stream

``` **main.js:** ```js let watcher; async function main() { // --> Replace with your secrets const APP_ID = ""; const DATA_SOURCE_NAME = ""; const DATABASE_NAME = ""; const COLLECTION_NAME = ""; // <-- const app = new Realm.App(APP_ID); const creds = Realm.Credentials.anonymous(); const user = await app.logIn(creds); console.log("User logged in: ", user.id); const mongo = app.currentUser.mongoClient(DATA_SOURCE_NAME); const collection = mongo.db(DATABASE_NAME).collection(COLLECTION_NAME); watcher = collection.watch(); console.log(watcher); for await (const change of watcher) { console.log("Change: ", change); } } async function stop() { console.log("stopping watcher"); await watcher.return(null); console.log("done"); } window.main = main; window.stop = stop; ```

As soon as I comment the block with native iterator check - the problem goes away.

UPD

We also noticed that after the return call:

Screenshot 2024-05-17 at 18 09 58
kraenhansen commented 5 months ago

@DeltaSPb I managed to reproduce this following your instructions, using Chrome for MacOS Version 125.0.6422.112 and 125.0.6422.113 (Official Build) (arm64). Thanks a lot for writing up the reproduction.

It took some time from pressing "Stop" until the crash, which makes me suspect the crash is related to garbage collection.

I'm currently trying to produce a stack trace from the minidump or build Chromium from source to attach LLDB.

kraenhansen commented 5 months ago

My current understanding of the problem is that Chrome added the Symbol.asyncIterable to the ReadableStream in recent versions, which makes the SDK take a different branch in asyncIteratorFromResponseBody.

Because of a bug in Chrome the for await...of syntax call next on the iterable after we've called return, which triggers this assertion in Chrome and cause the crash https://github.com/chromium/chromium/blob/9ff7a5644aafc13f10c8dc02e4141e85ff9b70f8/third_party/blink/renderer/bindings/core/v8/async_iterable.cc#L296 (I plan to report this issue with the Chrome browser some time in the future 🤞).

As @sharifmacky correctly hinted, removing the Symbol.asyncIterator in body branch from asyncIteratorFromResponseBody fix the issue and we actually need this, to ensure cancel is called on the ReadableStream (which ultimately cause the request to close).

Thanks a lot for reporting this and providing minimal reproductions!

kraenhansen commented 5 months ago

The fix was released through realm-web@v2.0.1 - please help take it for a spin. And thank you all for your patience on this one!

andy-dextrous commented 5 months ago

Thanks guys - appreciate it!