fix: block-dynamic-connections insertion marker monkey patch

[BeksOmega]

Description

Fixes #1448

Renames everything in the monkey patches to match what Rachel did in google/blockly#6602

Testing

Could not reproduce #1448

Additional Information

This is why monkey patches are sad =(

I think we could get this working in a non-monkey-patch way if we had some insertion-marker specific events.

[BeksOmega]

google/blockly#6602 hasn't been released yet, so as not to break the samples publish happening early Thursday, I'm going to wait to merge this until after we bump all the plugins to the Thursday core release.

[cpcallen]

Observed while debugging test failures: the first commit of this PR (that modifies the monkey patch) passes tests on its own, even without bumping blockly to 9.2.0. This is unexpected, since it references properties that are not introduced until 9.2.0.

@BeksOmega: Is the monkey patch itself not being tested?

[BeksOmega]

The monkey patch itself is not currently being tested, only the blocks' responses to onPendingConnection which is triggered by the patch. You can view some of the tests here: https://github.com/google/blockly-samples/blob/master/plugins/block-dynamic-connection/test/dynamic_if.mocha.js

[cpcallen]

OK, I have further information about what's causing the test failures.

Background

Here is the background information necessary to understand this problem—not all of which I understood when beginning to debug it.

jsdom

The jsdom npm module provides a pure-JavaScript implementation of the browser Document Object Model (DOM). It is useful useful in a number of situations:

• Since node.js does not implement browser APIs, jsdom can be used to polyfill the DOM.
  • This is what Blockly does, as its XML serialisation depends on the DOMParser and XMLSerializer` classes.
• It can also be used as a (limited-safety) sandbox for in-browser loading and manipulation of HTML documents, including running scripts against a "virtual" page.
• It would also be useful for frontend frameworks that use a virtual DOM, for those situations in which you have a surfeit of CPU time and you want to waste some of it by having your app make changes to a fake DOM and then have the framework copy those changes to the real DOM, as if we were all still building UIs using ncurses.

Polyfilling [[Internal Slot]]s

One issue that comes up when trying to polyfill built-ins (or in this case a "host-defined facility", such as the DOM), is that a lot of built-ins make use of [[Internal Slots]] to store information that can only be accessed via native code; a key feature of internal slots is that they are 'tamper-proof', and cannot be modified by manipulating the properties of the object, only by (native) methods. Notably, they are unaffected by whatever properties might exist (or be created) on the object.

For jsdom to be a faithful implementation of the DOM specification it needs to emulate internal slots in some way. There are various approaches to polyfilling internal slots; the way jsdom does this is using symbol-keyed properties.

In particular, the DOM classes (such as Node and Element) are actually lightweight wrappers around corresponding NodeImpl and ElementImpl objects that contain the internal slots and most of the actual method implementations. Each Node has a property keyed by a unique symbol which is set to the corresponding NodeImpl object containing the private data for that DOM—and similarly for Element -> ElementImpl and all the other DOM classes.

(Making it harder to figure out what was going on, the wrapper classes are defined in modules which are not actually part of the jsdom source repository but are generated during the build process.)

Webpack and the test runner

In the blockly-samples repository, our plugins are all tested via our @blockly/dev-scripts package which contains a test script which finds files matching tests/*.mocha.js and uses webpack to create a separate bundle for each one in the build/ directory.

Each bundle contains the test file and all of its dependencies, including a fully copy of blockly_compressed.js and jsdom. The eval-source-map devtool is used to pack each source file into the bundle in the form of an entry in an object literal whose key is the source filename and whose value is an arrow function that does a single eval of the original file's code transformed into roughly the format of CJS module with added sourcemap:

var __webpack_files__ = {
  'filename1': (module, exports, requires) => {
    eval(' /* module code: uses requires and modifies module/exports */ // /* sourcemap */');
   },
  // 'filename2': (...) ->{ ... },
  // ...
};

Debugging the test failures

As of the present time (commit d9f5e7d), the tests have four failures, all with similar errors; here is the first:

  1) If block
       Creation:
     TypeError: Failed to execute 'serializeToString' on 'XMLSerializer': parameter 1 is not of type 'Node'.
      at Object.exports.convert (webpack-internal:///./node_modules/jsdom/lib/jsdom/living/generated/Node.js:25:9)
      at XMLSerializer.serializeToString (webpack-internal:///./node_modules/jsdom/lib/jsdom/living/generated/XMLSerializer.js:111:23)
      at domToText$$module$build$src$core$utils$xml (webpack-internal:///./node_modules/blockly/blockly_compressed.js:74:333)
      at domToText$$module$build$src$core$xml (webpack-internal:///./node_modules/blockly/blockly_compressed.js:101:291)
      at saveExtraState$$module$build$src$core$serialization$blocks (webpack-internal:///./node_modules/blockly/blockly_compressed.js:188:459)
      at save$$module$build$src$core$serialization$blocks (webpack-internal:///./node_modules/blockly/blockly_compressed.js:187:4)
      at new BlockCreate$$module$build$src$core$events$events_block_create (webpack-internal:///./node_modules/blockly/blockly_compressed.js:597:600)
      at Block$$module$build$src$core$block.doInit_ (webpack-internal:///./node_modules/blockly/blockly_compressed.js:650:307)
      at new Block$$module$build$src$core$block (webpack-internal:///./node_modules/blockly/blockly_compressed.js:649:408)
      at Workspace$$module$build$src$core$workspace.newBlock (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1543:351)

From the stack trace, it is clear that an object of the wrong type was being passed to XMLSerializer.prototype.serializeToString(), and some initial testing locally suggested that this was being caused by the very first line of the first test:

  test('Creation', function() {
    const block = this.workspace.newBlock('dynamic_if');
    assertBlockStructure(block, ['IF0', 'DO0']);
  });

Initial probing using the node REPL

I made some initial attempts to load dynamic_if.mocha.js in node interactively; unfortunately this proved to be basically infeasible due node being very tetchy about loading ESM:

• You cannot import ... as foo from 'path' in the REPL, only const foo = await import('path').
• It will not load a file that contains an import statement if its extension is .js (rather than .mjs), unless the module's package.json contains "type": "module" (in which case all .js files in that package are treated as ES modules.)
• A symlink foo.mjs -> foo.js is not good enough to fool it; it sees the link target is a .js and responds "Ha ha ha, I see your tricks; no import for you!!", so you have to use hard links.
• You can't import ... from 'blockly/core', because it sees that there is a directory called core/ in the package and tells you you're not allowed to import a directory (rather than importing core.js as WebPack does).
• And import * as Blockly from 'blockly/core.js' doesn't work either, because in that case it treats the CJS module as having a single default export, so you have to change all the import statements to import default as Blockly from 'blockly/core.js' (or equivalently but more succinctly import Blockly from 'blockly/core.js'.)

Since all the relevant bits of code come from node_modules (much buried in blockly_compressed.js) and ends up in the webpacked bundle, printf debugging of npm test was going to be difficult, so I decided to try starting node with the --inspect option to allow me to connect to it with the Chrome developer tools as a debugger.

Getting the tests running in --inspect mode

It took a little while to figure out where to add the --inspect command because npm test runs several layers of node ... processes before the tests actually execute. It turns out that plugins/dev-scripts/scripts/test.js builds the test bundles then loads and executes them them, all directly within the same runtime; this script is run by plugins/dev-scripts/scripts/bin/blockly-scripts.js so I added --inspect (and also --inspect-brk for good measure) to that file:

 const script = args[0];
 if (availableScripts.includes(script)) {
-  const result = spawnSync('node',
-      [].concat(require.resolve('../scripts/' + script))
-          .concat(args.slice(1)), {stdio: 'inherit'}
-  );
+  const result = spawnSync('node', [
+    '--inspect', '--inspect-brk'
+  ].concat(require.resolve('../scripts/' + script))
+      .concat(args.slice(1)), {stdio: 'inherit'});

Unfortunately when the node process starts almost no code has been loaded, so it's not immediately possible to set breakpoints in our test code, blockly, or jsdom. The Chrome devtools will let you add files in the local filesystem to its inspector (under the "Filesystem" tab in the file browser sidebar) but setting breakpoints in them has no effect since the code actually being run by the tests is actually loaded from the bundle file (hence the webpack-internal: URLs in the stack trace above).

Fortunately by judicious use of step / next / etc. and setting breakpoints progressively further into the build / load / test sequence I was able to get the point where the files of interest had been loaded and were shown in the "Node" tab in the sidebar, at which point it became possible to add breakpoints to to dynamic_if.mocha.js as well as various parts of jsdom.

At this point I was able to trace through the code and definitively establish that the sequence of events is as follows:

• The test harness creates a Workspace and then calls .newBlock('dynamic_if') on it.
• The Block constructor is run.
• The constructor calls .doInit_
• .doInit_ runs eventUtils.fire(new (eventUtils.get(eventUtils.BLOCK_CREATE))(this))
• This results in a BlockCreate (event) instance being created.
• The BlockCreate constructor calls the save function in core/serialization/blocks.ts, passing it the newly-created block as an argument.
• This calls saveExtraState, again passing it the new block.
• Since the new block does not have a .saveExtraState method, the function instead calls .mutationToDom and then passes the resulting value (some XML Elements, hopefully) to the domToText function in core/xml.ts.
• domToText in core/xml.ts calls domToText in core/utils/xml.ts
• This other domToText creates an XMLSerializer instance and calls .serializeToSTring, passing it the XML.
• This blows up with a type error, complaining that the argument passed is not a Node (even though Element should extend Node).

What's going on in serializeToString?

The top-level jsdom method that throws is defined on the XMLSerializer class—a wrapper class—as follows (simplified; alas I cannot link to the original as this is a generated file):

    serializeToString(root) {
      // Sanity checking value of this omitted.

      if (arguments.length < 1) {
        throw new globalObject.TypeError( /* one argument required */);
      }
      const args = [];
      args.push(Node.convert(globalObject, curArg, { /* Error message if conversion fails. */ }));
      return this[implSymbol].serializeToString(...args);
    }

Basically this just does some type checking, "converts" its argument, then uses unique symbol (in the implSymbol) to get the corresponding XMLSerializerImpl instance that will actually do the .serializeToString work.

Evidently Node.convert (a static method) is throwing. What is this method? It is simple:

exports.convert = (globalObject, value, { context = "The provided value" } = {}) => {
  if (exports.is(value)) {
    return utils.implForWrapper(value);
  }
  throw new globalObject.TypeError(`${context} is not of type 'Node'.`);
};

This is throwing because exports.is returns false. What is that? Also simple:

exports.is = value => {
  return utils.isObject(value) && utils.hasOwn(value, implSymbol) && value[implSymbol] instanceof Impl.implementation;
};

This checks to make sure its argument is actually an object, and if it is it uses the implSymbol as a key to unlock the property that contains the reference to the argument's actual implementation (private data) object, which it then checks to make sure is an instance of Impl.implementation which is (in this case) just NodeImpl. So convert will throw a type error if it is passed

• a non-object, or
• an object which does not have an implementation property, or
• a wrapper object whose private implementation object is not a NodeImple.

The debugger, however, shows that the value passed at this point is indeed an Element (which extends Node), and it has a [Symbol('impl')] property whose value is an ElementImpl object (which is an instance of NodeImpl).

More than one implSymbol?

The curious thing is that, although the debugger showed the presence of a property whose key was Symbol('impl'), checking value[implSymbol] returned undefined. They must be two different symbols!

This was confirmed by extracting the symbol from the object and comparing it with implSymbol: Object.getOwnPropertySymbols(value)[0] === implSymbol returned false, even though both values were printed as Symbol('impl').

But all implSymbols in jsdom come from a single module, lib/jsdom/living/generated/utils.js, which defines a single const implSymbol = Symbol("impl"). Moreover, this file should only be evaluated once; subsequent require or imports should return the previously-created exports / Module object without re-evaluating the module's source code. Yet it appeared it was being run more than once!

Initially I spent a bunch of time trying to see if more than one copy of jsdom was being included in the build. In some circumstances it appeared that more than one copy of Blockly was: I could see both node_modules/blockly/ and node_modules/@blockly/dev-tools/blockly/ (the latter being v9.0.0), but I was unable to locate a second copy of jsdom.

Finally, adding a breakpoint on the line where implSymbol is defined revealed that it was in fact being run three times. This lead me to realise that a separate copy was included in each of the build/<test>.mocha.js bundles, and (thanks to to the inline sourcemaps provided by the aforementioned eval-source-map tool) the Chrome inspector was stopping at the same point in each one even though I had created but a single breakpoint.

Finally, to verify exactly what was going on, I changed the line where implSymbol was defined to read:

const implSymbol = Symbol("impl " + require('path').basename(__filename));

This inserts the actual filename (in this case, of the bundle) into the symbol tag.

Now, re-running the tests I could see that (at the first failure, in dynamic_if.mocha.js) serialzeToString is being passed an Element keyed with the symbol generated when loading the bundle of the same name, but for some reason exports.is is using an implSymbol that was created when the dynamic_text_join.mocha.js bundle was loaded!

At this point I began writing up this discussion; in the process the last few pieces of the puzzle would fall into place.

Why the mismatch?

One of the key advantages of having separate bundles for each .mocha.js test file is supposed to be test isolation: each bundle should be completely independent of the others, containing all its own dependencies. This means that, even though they are all loaded into the same JavaScript runtime in sequence they should all be completely isolated from each other.

Yet clearly that was not the case. Why not? A bug in webpack seemed unlikely; instead I got to thinking about the one bit of code that runs when the blockly/core module is loaded in node.js but not in a browser—the bit that loads jsdom if needed:

// Override textToDomDocument and provide Node.js alternatives to DOMParser and
// XMLSerializer.
if (typeof globalThis.document !== 'object') {
  const {JSDOM} = require('jsdom');
  const {window} = new JSDOM(`<!DOCTYPE html>`);
  globalThis.DOMParser = window.DOMParser;
  globalThis.XMLSerializer = window.XMLSerializer;
  const xmlDocument = Blockly.utils.xml.textToDomDocument(
      `<xml xmlns="${Blockly.utils.xml.NAME_SPACE}"></xml>`);
  Blockly.utils.xml.setDocument(xmlDocument);
}

Notably, this code uses the jsdom module to create and save for later use three things:

1. A DOMParser constructor, which is stored in a global variable,
2. An XMLSerializer constructor, which is also stored in a global variable, and
3. A Document named xmlDocument, which is saved by passing it to Blockly.utils.xml.setDocument().

All of these are used (only) in core/utils/xml.ts, and nowhere else (except via that module).

Thinking further about this, however, I realised that, if this file were to be run more than once (as is the case here, due to separate copies of it being included in each test bundle), each copy of the blockly module would get its own separate xmlDocument, but in then end they would all share the same DOMParser and XMLSerializer constructors due to the use of global variables.

That's fine for the last-loaded copy of Blockly, but the earlier ones would naturally run into problems when XMLSerializer failed to recognise Node instances generated by their earlier xmlDocument, whose methods (notably, .createElementNS) would be using a different implSymbol.

Scope of the problem

Based on what we presently know:

• This issue only came up due to the jsdom upgrade in google/blockly#6591, which bumped the package from 15.2.1 to 20.0.2. Presumably the older version was less meticulous in its type-checking, or used a different strategy to hide internal data.
• It will not affect Blockly run in a browser, as that does not load jsdom but uses the native implementations.
• It will not affect any normal usage of Blockly in node.js, where the blockly/core and jsdom modules should only be loaded once each.
• It will only manifest if multiple copies of blockly are loaded in the same JavaScript runtime, and that runtime is node.js (or another that does not have a native DOM implementation).
• It will affect most/all of our plugins (once they are updated to use 9.2.0), as well as any external plugin developers who use the @blockly/dev-scripts package and have more than one .mocha.js test file.

How to fix?

There are various possible approaches:

1. Change our test scripts so that they do not create multiple copies of Blockly and its dependencies.
   • Running the .mocha.js files directly, rather than first bundling them, would also make the tests run considerably faster as the preponderance of the time taken to run tests is spent running webpack.
2. Change the test script to run each bundle in a separate node instance.
3. Change the plugin tests to have at most one test/*.mocha.js file per plugin.
4. Fix Blockly's scripts/package/node/core.js and core/utils/xml.ts so that all three required bits of the DOM are handled the same way (preferably not via global variables).

I suggest that option 4 be pursued in the short term, with option 1 to be considered in future.