[parser] Avoid processing empty preparse data objects

Always precheck that the PreparseData has data before serializing it.

Drive-by-fix:
- rename preparsed_scope_data_builder_ to preparse_data_builder_

Change-Id: I8e709af8f69db44e03caa9132f06a7b8c906bfdb
Reviewed-on: https://chromium-review.googlesource.com/c/1387305Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58635}

src/objects.cc

@@ -14459,18 +14459,16 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
     shared_info->set_length(SharedFunctionInfo::kInvalidLength);
     ProducedPreparseData* scope_data = lit->produced_preparse_data();
     if (scope_data != nullptr) {
-      Handle<PreparseData> preparse_data;
-      if (scope_data->Serialize(shared_info->GetIsolate())
-              .ToHandle(&preparse_data)) {
+      Handle<PreparseData> preparse_data =
+          scope_data->Serialize(shared_info->GetIsolate());
       Handle<UncompiledData> data =
           isolate->factory()->NewUncompiledDataWithPreparseData(
-                lit->inferred_name(), lit->start_position(),
-                lit->end_position(), lit->function_literal_id(), preparse_data);
+              lit->inferred_name(), lit->start_position(), lit->end_position(),
+              lit->function_literal_id(), preparse_data);
       shared_info->set_uncompiled_data(*data);
       needs_position_info = false;
     }
   }
-  }
   if (needs_position_info) {
     Handle<UncompiledData> data =
         isolate->factory()->NewUncompiledDataWithoutPreparseData(

src/parsing/parser.cc

@@ -2567,6 +2567,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
   bool did_preparse_successfully =
       should_preparse && SkipFunction(function_name, kind, function_type, scope,
                                       &num_parameters, &produced_preparse_data);
+
   if (!did_preparse_successfully) {
     // If skipping aborted, it rewound the scanner until before the LPAREN.
     // Consume it in that case.

src/parsing/preparse-data.cc

@@ -152,7 +152,6 @@ PreparseDataBuilder::PreparseDataBuilder(Zone* zone,
       byte_data_(new (zone) ByteData(zone)),
       data_for_inner_functions_(zone),
       bailed_out_(false) {
-  if (parent != nullptr) parent->data_for_inner_functions_.push_back(this);
 #ifdef DEBUG
   // Reserve space for scope_data_start, written later:
   byte_data_->WriteUint32(0);
@@ -170,7 +169,12 @@ void PreparseDataBuilder::DataGatheringScope::Start(
 
 PreparseDataBuilder::DataGatheringScope::~DataGatheringScope() {
   if (builder_ == nullptr) return;
-  preparser_->set_preparsed_scope_data_builder(builder_->parent_);
+  PreparseDataBuilder* parent = builder_->parent_;
+  if (parent != nullptr) {
+    parent->data_for_inner_functions_.push_back(builder_->HasData() ? builder_
+                                                                    : nullptr);
+  }
+  preparser_->set_preparsed_scope_data_builder(parent);
 }
 
 void PreparseDataBuilder::AddSkippableFunction(int start_position,
@@ -225,10 +229,13 @@ bool PreparseDataBuilder::ContainsInnerFunctions() const {
   return byte_data_->size() > ByteData::kPlaceholderSize;
 }
 
-MaybeHandle<PreparseData> PreparseDataBuilder::Serialize(Isolate* isolate) {
-  if (bailed_out_) return MaybeHandle<PreparseData>();
+bool PreparseDataBuilder::HasData() const {
+  return !bailed_out_ && ContainsInnerFunctions();
+}
+
+Handle<PreparseData> PreparseDataBuilder::Serialize(Isolate* isolate) {
+  DCHECK(HasData());
   DCHECK(!ThisOrParentBailedOut());
-  if (!ContainsInnerFunctions()) return MaybeHandle<PreparseData>();
 
   int child_data_length = static_cast<int>(data_for_inner_functions_.size());
   Handle<PreparseData> data =
@@ -239,8 +246,8 @@ MaybeHandle<PreparseData> PreparseDataBuilder::Serialize(Isolate* isolate) {
 
   int i = 0;
   for (const auto& item : data_for_inner_functions_) {
-    Handle<PreparseData> child_data;
-    if (item->Serialize(isolate).ToHandle(&child_data)) {
+    if (item != nullptr) {
+      Handle<PreparseData> child_data = item->Serialize(isolate);
       data->set_child_data(i, *child_data);
     } else {
       DCHECK(data->child_data(i)->IsNull());
@@ -252,9 +259,8 @@ MaybeHandle<PreparseData> PreparseDataBuilder::Serialize(Isolate* isolate) {
 }
 
 ZonePreparseData* PreparseDataBuilder::Serialize(Zone* zone) {
-  if (bailed_out_) return nullptr;
+  DCHECK(HasData());
   DCHECK(!ThisOrParentBailedOut());
-  if (!ContainsInnerFunctions()) return nullptr;
 
   int child_length = static_cast<int>(data_for_inner_functions_.size());
   ZonePreparseData* result =
@@ -262,8 +268,10 @@ ZonePreparseData* PreparseDataBuilder::Serialize(Zone* zone) {
 
   int i = 0;
   for (const auto& item : data_for_inner_functions_) {
+    if (item != nullptr) {
       ZonePreparseData* child = item->Serialize(zone);
       result->set_child(i, child);
+    }
     i++;
   }
 
@@ -366,9 +374,11 @@ void PreparseDataBuilder::SaveDataForInnerScopes(Scope* scope) {
 class BuilderProducedPreparseData final : public ProducedPreparseData {
  public:
   explicit BuilderProducedPreparseData(PreparseDataBuilder* builder)
-      : builder_(builder) {}
+      : builder_(builder) {
+    DCHECK(builder->HasData());
+  }
 
-  MaybeHandle<PreparseData> Serialize(Isolate* isolate) final {
+  Handle<PreparseData> Serialize(Isolate* isolate) final {
     return builder_->Serialize(isolate);
   }
 
@@ -385,7 +395,10 @@ class OnHeapProducedPreparseData final : public ProducedPreparseData {
   explicit OnHeapProducedPreparseData(Handle<PreparseData> data)
       : data_(data) {}
 
-  MaybeHandle<PreparseData> Serialize(Isolate* isolate) final { return data_; }
+  Handle<PreparseData> Serialize(Isolate* isolate) final {
+    DCHECK(!data_->is_null());
+    return data_;
+  }
 
   ZonePreparseData* Serialize(Zone* zone) final {
     // Not required.
@@ -400,7 +413,7 @@ class ZoneProducedPreparseData final : public ProducedPreparseData {
  public:
   explicit ZoneProducedPreparseData(ZonePreparseData* data) : data_(data) {}
 
-  MaybeHandle<PreparseData> Serialize(Isolate* isolate) final {
+  Handle<PreparseData> Serialize(Isolate* isolate) final {
     return data_->Serialize(isolate);
   }
 

src/parsing/preparse-data.h

@@ -115,6 +115,7 @@ class PreparseDataBuilder : public ZoneObject {
 #endif  // DEBUG
 
   bool ContainsInnerFunctions() const;
+  bool HasData() const;
 
   static bool ScopeNeedsData(Scope* scope);
   static bool ScopeIsSkippableFunctionScope(Scope* scope);
@@ -126,7 +127,7 @@ class PreparseDataBuilder : public ZoneObject {
  private:
   friend class BuilderProducedPreparseData;
 
-  MaybeHandle<PreparseData> Serialize(Isolate* isolate);
+  Handle<PreparseData> Serialize(Isolate* isolate);
   ZonePreparseData* Serialize(Zone* zone);
 
   void SaveDataForScope(Scope* scope);
@@ -149,7 +150,7 @@ class ProducedPreparseData : public ZoneObject {
   // If there is data (if the Scope contains skippable inner functions), move
   // the data into the heap and return a Handle to it; otherwise return a null
   // MaybeHandle.
-  virtual MaybeHandle<PreparseData> Serialize(Isolate* isolate) = 0;
+  virtual Handle<PreparseData> Serialize(Isolate* isolate) = 0;
 
   // If there is data (if the Scope contains skippable inner functions), return
   // an off-heap ZonePreparseData representing the data; otherwise

src/parsing/preparser.cc

@@ -132,13 +132,12 @@ PreParser::PreParseResult PreParser::PreParseFunction(
 
   // Start collecting data for a new function which might contain skippable
   // functions.
-  PreparseDataBuilder::DataGatheringScope preparsed_scope_data_builder_scope(
-      this);
+  PreparseDataBuilder::DataGatheringScope preparse_data_builder_scope(this);
 
   if (IsArrowFunction(kind)) {
     formals.is_simple = function_scope->has_simple_parameters();
   } else {
-    preparsed_scope_data_builder_scope.Start(function_scope);
+    preparse_data_builder_scope.Start(function_scope);
 
     // Parse non-arrow function parameters. For arrow functions, the parameters
     // have already been parsed.
@@ -227,8 +226,10 @@ PreParser::PreParseResult PreParser::PreParseFunction(
 
       DeclareFunctionNameVar(function_name, function_type, function_scope);
 
+      if (preparse_data_builder_->HasData()) {
         *produced_preparse_data =
-          ProducedPreparseData::For(preparsed_scope_data_builder_, main_zone());
+            ProducedPreparseData::For(preparse_data_builder_, main_zone());
+      }
     }
 
     if (pending_error_handler()->has_error_unidentifiable_by_preparser()) {
@@ -286,12 +287,11 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
   // Start collecting data for a new function which might contain skippable
   // functions.
   {
-    PreparseDataBuilder::DataGatheringScope preparsed_scope_data_builder_scope(
-        this);
+    PreparseDataBuilder::DataGatheringScope preparse_data_builder_scope(this);
     skippable_function = !function_state_->next_function_is_likely_called() &&
-                         preparsed_scope_data_builder_ != nullptr;
+                         preparse_data_builder_ != nullptr;
     if (skippable_function) {
-      preparsed_scope_data_builder_scope.Start(function_scope);
+      preparse_data_builder_scope.Start(function_scope);
     }
 
     FunctionState function_state(&function_state_, &scope_, function_scope);
@@ -338,7 +338,7 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
   }
 
   if (skippable_function) {
-    preparsed_scope_data_builder_->AddSkippableFunction(
+    preparse_data_builder_->AddSkippableFunction(
         function_scope->start_position(), end_position(),
         function_scope->num_parameters(), GetLastFunctionLiteralId() - func_id,
         function_scope->language_mode(), function_scope->NeedsHomeObject());
@@ -382,19 +382,19 @@ PreParserBlock PreParser::BuildParameterInitializationBlock(
   DCHECK(!parameters.is_simple);
   DCHECK(scope()->is_function_scope());
   if (scope()->AsDeclarationScope()->calls_sloppy_eval() &&
-      preparsed_scope_data_builder_ != nullptr) {
+      preparse_data_builder_ != nullptr) {
     // We cannot replicate the Scope structure constructed by the Parser,
     // because we've lost information whether each individual parameter was
     // simple or not. Give up trying to produce data to skip inner functions.
-    if (preparsed_scope_data_builder_->parent() != nullptr) {
+    if (preparse_data_builder_->parent() != nullptr) {
       // Lazy parsing started before the current function; the function which
       // cannot contain skippable functions is the parent function. (Its inner
       // functions cannot either; they are implicitly bailed out.)
-      preparsed_scope_data_builder_->parent()->Bailout();
+      preparse_data_builder_->parent()->Bailout();
     } else {
       // Lazy parsing started at the current function; it cannot contain
       // skippable functions.
-      preparsed_scope_data_builder_->Bailout();
+      preparse_data_builder_->Bailout();
     }
   }
 

src/parsing/preparser.h

@@ -993,7 +993,7 @@ class PreParser : public ParserBase<PreParser> {
                               runtime_call_stats, logger, script_id,
                               parsing_module, parsing_on_main_thread),
         use_counts_(nullptr),
-        preparsed_scope_data_builder_(nullptr) {}
+        preparse_data_builder_(nullptr) {}
 
   static bool IsPreParser() { return true; }
 
@@ -1020,12 +1020,12 @@ class PreParser : public ParserBase<PreParser> {
       ProducedPreparseData** produced_preparser_scope_data, int script_id);
 
   PreparseDataBuilder* preparsed_scope_data_builder() const {
-    return preparsed_scope_data_builder_;
+    return preparse_data_builder_;
   }
 
   void set_preparsed_scope_data_builder(
       PreparseDataBuilder* preparsed_scope_data_builder) {
-    preparsed_scope_data_builder_ = preparsed_scope_data_builder;
+    preparse_data_builder_ = preparsed_scope_data_builder;
   }
 
  private:
@@ -1719,7 +1719,7 @@ class PreParser : public ParserBase<PreParser> {
   int* use_counts_;
   PreParserLogger log_;
 
-  PreparseDataBuilder* preparsed_scope_data_builder_;
+  PreparseDataBuilder* preparse_data_builder_;
 };
 
 PreParserExpression PreParser::SpreadCall(const PreParserExpression& function,