xvrh / puppeteer-dart

A Dart library to automate the Chrome browser over the DevTools Protocol. This is a port of the Puppeteer API
BSD 3-Clause "New" or "Revised" License
236 stars 59 forks source link

Check for null child node/Ids #224

Closed philipbalvanz-wf closed 1 year ago

philipbalvanz-wf commented 1 year ago

I've been seeing the following error intermittently when running puppeteer-dart, which is causing test run failures.

Null check operator used on a null value
package:puppeteer/src/page/accessibility.dart 548:53  _AXNode.createTree
package:puppeteer/src/page/accessibility.dart 69:31   Accessibility.snapshot

I was able to recreate this scenario with the following code:

class AXNodeId {
  final String value;
  AXNodeId(this.value);

  factory AXNodeId.fromJson(String value) => AXNodeId(value);
}

class AXNodeData {
  final List<AXNodeId>? childIds;

  AXNodeData({this.childIds});

  factory AXNodeData.fromJson(Map<String, dynamic> json) {
    return AXNodeData(
        childIds: json.containsKey('childIds')
            ? (json['childIds'] as List)
            .map((e) => AXNodeId.fromJson(e as String))
            .toList()
            : null
    );
  }
}

class _AXNode {
  final AXNodeData _payload;
  final _children = <_AXNode>[];
  _AXNode(this._payload);
}

void main() {
  final nodeData = AXNodeData.fromJson({'childIds':['123']});
  var nodeById = <String, _AXNode>{};
  final axNode = _AXNode(nodeData);

  for(var childId in axNode._payload.childIds!){
    axNode._children.add(nodeById[childId.value]!);
  }
}

The issue appears to be related to the use of the null assertion operator (!) on a value that can in fact be null in this case. From what I can tell nodeById does not contain the childId.value in the map, thus resulting in null being returned. This appears to indicate that the child node is missing? from the FullAXTree(removed during processing?) or we've been given a bad id. Let me know if the FullAXTree missing a node is cause for concern.

My proposal is to explicitly check for null after retrieving the value, prior to adding it to the list of children. I also updated the node._payload.childIds! to prevent the nesting from getting any deeper.

Overall it feels like the null assertion operator ! should be used sparingly, as the analyzer simply shuts off, even if you didn't check for null first.

@xvrh for your review

xvrh commented 1 year ago

Thanks for the contribution!