Fix TextDecoder exception on decoding SharedArrayBuffer

[jiulongw]

Fixes #15217

This patch is used in our product to fix a problem described in #15217.

I haven't run a full browser compatibility tests other than Chrome. But I see this pattern (type checking based on Symbol.toStringTag) already being used so I guess it is acceptable.

As for performance, benchmark shows Object.prototype.toString.call is slightly faster than instanceof.

[jiulongw]

Update: It seems this bug only affects Google Chrome when WASM is loaded by localhost. Not a high priority issue.

[mattmone]

Update: It seems this bug only affects Google Chrome when WASM is loaded by localhost. Not a high priority issue.

I have experienced this not on localhost and it breaks some future functionality of our app quite consistently

[tthef]

This issue is not limited to localhost.

[sbc100]

This issue is not limited to localhost.

Can you describe how/when you are able to reproduce the issue? Any idea what set of circumstances cause it?

[tthef]

This issue is not limited to localhost.

Can you describe how/when you are able to reproduce the issue? Any idea what set of circumstances cause it?

Use of embind and iframes, as per the original description in the linked issue #15217.

[jiulongw]

This issue is not limited to localhost.

Can you describe how/when you are able to reproduce the issue? Any idea what set of circumstances cause it?

Use of embind and iframes, as per the original description in the linked issue #15217.

Right. This not only happen in localhost, but in our production environment as well, in latest Chrome (m109). In our case, it happens when a tab is being duplicated. Using -s TEXTDECODER=0 can work around this problem with a bit speed penalty, as mentioned in #18034.

[tthef]

We have been patching our toolchain with this PR's patch for some time now, works well.

[kripken]

If this is useful for people then we should think about landing it. But first I'd like to know if this is a workaround for a current bug or if it is known behavior. A link to a chrome/firefox/safari issue would be useful, or to some other relevant docs on MDN or such. Or a testcase so that we can file an issue.

[tthef]

If this is useful for people then we should think about landing it. But first I'd like to know if this is a workaround for a current bug or if it is known behavior. A link to a chrome/firefox/safari issue would be useful, or to some other relevant docs on MDN or such. Or a testcase so that we can file an issue.

It's not a workaround, it's a bug in emscripten due to incorrect assumptions about the instanceOf operator, all the relevant information, including a minimal code sample, is in the original ticket, #15217.

[juj]

It's not a workaround, it's a bug in emscripten

One person's bug is another person's unsupported use case.

I agree we should add support for these types of scenarios. This PR is still pending addressing the review points 1. - 4. in https://github.com/emscripten-core/emscripten/pull/16994#discussion_r960379197 , which to summarize has the following concerns:

• we don't have testing for this use case, so it is clear that this use case has gone unnoticed,
• there are more instances of "incorrect assumptions" in Emscripten that also need to be modified for splitting iframes like this to work - or to be reasoned that those conditions cannot occur anywhere else? (if so, why not?) In the absence, this PR seems partial to fix one code flow path in one program, not to add general support for this kind of split iframe use case, (https://github.com/emscripten-core/emscripten/issues/15217#issuecomment-1132412745 suggests that there are other call sites as well that need tending to)
• The standardization over .toString() format is still a question: in the past I have seen .toString() calls return different syntax on some types in different browsers/shells. Would e.g. using x.constructor.name serve the same purpose, which is guaranteed to have a unified result?

Ultimately I think we should add this as a compile-time feature and do something like {{{ isInstanceOf(x, y) }}} which would get conditionally compiled to a fast instanceOf() call for people who are building binaries that don't split iframe domains, and then the more general check for people who opt in to it. (to apply zero-overhead principle)

For all that to happen, there would have to be someone who cares about this use case to provide such an improvement. In the current form this PR would regress users who don't utilize this kind of deployment, and there is no testing visibility to ensure that it wouldn't break in the future.

[arsnyder16]

@sbc100 @juj @kripken Any update here I am encountering this as well when using localhost. Don't believe i have seen this in any production but its a pain point when trying to locally debug without needing to specific options (-s TEXTDECODER=0) just for local that aren't used in production

[sbc100]

@sbc100 @juj @kripken Any update here I am encountering this as well when using localhost. Don't believe i have seen this in any production but its a pain point when trying to locally debug without needing to specific options (-s TEXTDECODER=0) just for local that aren't used in production

@arsnyder16 do you know what you are doing that triggers this bug when testing locally? I'm trying to figure out why we don't see this issue in any of our testing.

[arsnyder16]

@sbc100 Nothing obvious, we have been using emscripten for a few years, and part of our CI process runs GTests through emrun that run quite a bit of code and i don't think we ever saw this in this environment. I just happened to be debugging one of our tests locally today and i started to hit this.

If i see a pattern or find explanation/theory i will certainly keep you updated.

[sbc100]

@sbc100 Nothing obvious, we have been using emscripten for a few years, and part of our CI process runs GTests through emrun that run quite a bit of code and i don't think we ever saw this in this environment. I just happened to be debugging one of our tests locally today and i started to hit this.

If i see a pattern or find explanation/theory i will certainly keep you updated.

Can you share the full set of link flags you use?

[arsnyder16]

Sure

-sINITIAL_MEMORY=100MB 
-sSTACK_SIZE=2MB 
-fexceptions 
-sWASM_BIGINT 
-sALLOW_MEMORY_GROWTH 
-sMALLOC=mimalloc 
-sEXIT_RUNTIME 
--post-js forward-env.js 
-pthread 
-sPROXY_TO_PTHREAD 
-Os 
--emrun 
-sENVIRONMENT=web,worker 
-sLZ4 
-sFETCH 
--extern-post-js bulk-console-print.js 
--preload-file=Resources@Resources 
--preload-file=Testing@Testing

I don't think they have any influence here, but the custom js we are adding with extern-post-js and post-js is to workaround a few issues here they are

forward-env.js

#16517

if(ENVIRONMENT_IS_NODE) {
  ENV = Object.create(process.env);
} else if(ENVIRONMENT_IS_WEB)  {
  ENV = Module['arguments'].reduce((prev, curr) => {
    if (curr.startsWith('--env=')) {
      const results = curr.split('=');
      prev[results[1]] = results[2];
    }
    return prev
  }, {
    "TZ": Intl.DateTimeFormat().resolvedOptions().timeZone // https://github.com/emscripten-core/emscripten/pull/16517 
  });
}

bulk-console-print.js

#15839

// https://github.com/emscripten-core/emscripten/issues/15839
if (!Module["ENVIRONMENT_IS_PTHREAD"] && (arguments_.includes('--gtest_list_tests') || !arguments_.includes('--no-console-buffer'))) {
   var flushs = [];
   var flushConsole = () => flushs.forEach(x => x());
   var flushInterval;
   var resetInterval = () => {
      clearInterval(flushInterval);
      flushInterval = setInterval(flushConsole, 60 * 1000);
   };
   resetInterval();
   var processConsoleLine = cb => {
      var lines = [];
      var limit = 1000;
      var flush = () => {
         resetInterval();
         if (!lines.length) {
            return;
         }
         cb(lines.join('\n'));
         lines = [];
      };
      flushs.push(flush);
      return (text) => {
         lines.push(text);
         lines.length > limit && flush();
      };
   }
   out = processConsoleLine(out);
   err = out;
   addOnExit(flushConsole);
}

[sbc100]

Sure

-sINITIAL_MEMORY=100MB 
-sSTACK_SIZE=2MB 
-fexceptions 
-sWASM_BIGINT 
-sALLOW_MEMORY_GROWTH 
-sMALLOC=mimalloc 
-sEXIT_RUNTIME 
--post-js forward-env.js 
-pthread 
-sPROXY_TO_PTHREAD 
-Os 
--emrun 
-sENVIRONMENT=web,worker 
-sLZ4 
-sFETCH 
--extern-post-js bulk-console-print.js 
--preload-file=Resources@Resources 
--preload-file=Testing@Testing

I don't think they have any influence here, but the custom js we are adding with extern-post-js and post-js is to workaround a few issues here they are

forward-env.js bulk-console-print.js

If there is some way you could reduce this something smaller, perhaps using the simple test/hello_world.c program that would be great. Or perhaps you could try to figure out which of those settings is triggering this issue?

What browser are you using BTW?

[arsnyder16]

If there is some way you could reduce this something smaller, perhaps using the simple test/hello_world.c program that would be great. Or perhaps you could try to figure out which of those settings is triggering this issue?

What browser are you using BTW?

Chrome 122.

One random theory. Since we are using memory growth and pthreads. Is it possible that during a UTF8ArrayToString another thread is causing a wasmMemory.grow to happen ?

• Can a grow call happen on a worker thread?
  • if so will other thread experience the instanceof issue?
  • What happens when that thread is destoryed?

I tested this theory very loosely by changing -sINITIAL_MEMORY=100MB to -sINITIAL_MEMORY=1500MB in my setup to try and avoid an memory growing, and i no longer see the problem. So it seems related

My guess is an example with multiple threads that are requiring more memory to be allocated and interleaved with some threads triggering UTF8ArrayToString may simulate the problem

pseudo related to dealing with GROWABLE_HEAP etc

[sbc100]

If there is some way you could reduce this something smaller, perhaps using the simple test/hello_world.c program that would be great. Or perhaps you could try to figure out which of those settings is triggering this issue? What browser are you using BTW?

Chrome 122.

One random theory. Since we are using memory growth and pthreads. Is it possible that during a UTF8ArrayToString another thread is causing a wasmMemory.grow to happen ?

• Can a grow call happen on a worker thread?

  • if so will other thread experience the instanceof issue?
  • What happens when that thread is destoryed?

I tested this theory very loosely by changing -sINITIAL_MEMORY=100MB to -sINITIAL_MEMORY=1500MB in my setup to try and avoid an memory growing, and i no longer see the problem. So it seems related

My guess is an example with multiple threads that are requiring more memory to be allocated and interleaved with some threads triggering UTF8ArrayToString may simulate the problem

pseudo related to dealing with GROWABLE_HEAP etc

This seems like it might be an unrelated issue then.

Can you catch the exception in a debugger and see what ${heap}.buffer is at the point failure?

Can you confirm is this patch fixes your issue or not?

[arsnyder16]

I am certain this PR would fix the crash based on the output below from the browser console. An explanation of the exact behavior or reason instanceof stops working is still unclear but i think my theory of growing etc is related to producing the situation. It doesn't seem strictly related to iframes.

[kripken]

@arsnyder16

My guess is an example with multiple threads that are requiring more memory to be allocated and interleaved with some threads triggering UTF8ArrayToString may simulate the problem

I tried to test that theory but can't seem to hit the problem:

https://gist.github.com/kripken/170c8cac2660264e6563104a2fb3f63f

$ emcc a.cpp -pthread -O3 -sPROXY_TO_PTHREAD -sALLOW_MEMORY_GROWTH -sEXIT_RUNTIME --profiling -o a.html
$ emrun a.html

This seems to be a browser bug of some kind so it would really be good to get a reproducer so we can file an issue. Even if we want to land this workaround for now until it is fixed - which I am ok with in principle - it would be good to confirm the problem exists and is fixed by it.