Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sharifmacky opened this issue Apr 21, 2024 · 19 comments · Fixed by #6688
Closed

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

sharifmacky opened this issue Apr 21, 2024 · 19 comments · Fixed by #6688
Assignees
Labels
O-Community Waiting-For-Reporter Waiting for more information from the reporter before we can proceed

Comments

@sharifmacky
Copy link

sharifmacky commented Apr 21, 2024

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

Copy link

sync-by-unito bot commented Apr 21, 2024

➤ PM Bot commented:

Jira ticket: RJS-2809

@kneth
Copy link
Contributor

kneth commented Apr 22, 2024

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?

@sync-by-unito sync-by-unito bot added the Waiting-For-Reporter Waiting for more information from the reporter before we can proceed label Apr 22, 2024
@sharifmacky
Copy link
Author

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
Copy link

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.

@github-actions github-actions bot added Needs-Attention Reporter has responded. Review comment. and removed Waiting-For-Reporter Waiting for more information from the reporter before we can proceed labels Apr 23, 2024
@sharifmacky
Copy link
Author

} else if (Symbol.asyncIterator in body) {
return body as AsyncIterable<Uint8Array>;

@andy-dextrous
Copy link

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
Copy link
Contributor

kneth commented Apr 26, 2024

@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.

@sync-by-unito sync-by-unito bot removed the Needs-Attention Reporter has responded. Review comment. label Apr 26, 2024
@sharifmacky sharifmacky changed the title calling stream.return() on mobile chrome crashes browser calling stream.return() on chrome 124 crashes browser Apr 30, 2024
@piesuke
Copy link

piesuke commented May 7, 2024

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

@kneth
Copy link
Contributor

kneth commented May 7, 2024

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

@mevilbhojani
Copy link

Our team also face similar issue .

@kneth
Copy link
Contributor

kneth commented May 15, 2024

@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).

@sync-by-unito sync-by-unito bot added the Waiting-For-Reporter Waiting for more information from the reporter before we can proceed label May 15, 2024
@DeltaSPb
Copy link

DeltaSPb commented May 15, 2024

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
Copy link

@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
Copy link
Contributor

kneth commented May 16, 2024

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
Copy link

DeltaSPb commented May 17, 2024

@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:

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

  <body>
    <h1>Change stream</h1>
    <button id="startButton" type="button" onclick="main()">Start</button>
    <button id="stopButton" type="button" onclick="stop()">Stop</button>
  </body>
</html>

main.js:

let watcher;

async function main() {
  // --> Replace with your secrets
  const APP_ID = "<my secret>";
  const DATA_SOURCE_NAME = "<my secret>";
  const DATABASE_NAME = "<my secret>";
  const COLLECTION_NAME = "<some 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:

  • crash in Chrome occurs as soon as execution starts in the loop body
  • some exception in the Firefox:
Screenshot 2024-05-17 at 18 09 58

@kraenhansen
Copy link
Member

kraenhansen commented May 28, 2024

@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
Copy link
Member

kraenhansen commented May 29, 2024

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
Copy link
Member

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
Copy link

Thanks guys - appreciate it!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
O-Community Waiting-For-Reporter Waiting for more information from the reporter before we can proceed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants