test: move more web contents specs (#20099)

shell/browser/api/atom_api_web_contents.cc

@@ -933,10 +933,14 @@ void WebContents::DidFinishLoad(content::RenderFrameHost* render_frame_host,
   bool is_main_frame = !render_frame_host->GetParent();
   int frame_process_id = render_frame_host->GetProcess()->GetID();
   int frame_routing_id = render_frame_host->GetRoutingID();
+  auto weak_this = GetWeakPtr();
   Emit("did-frame-finish-load", is_main_frame, frame_process_id,
        frame_routing_id);
 
-  if (is_main_frame)
+  // ⚠️WARNING!⚠️
+  // Emit() triggers JS which can call destroy() on |this|. It's not safe to
+  // assume that |this| points to valid memory at this point.
+  if (is_main_frame && weak_this)
     Emit("did-finish-load");
 }
 
@@ -1092,6 +1096,9 @@ void WebContents::DidFinishNavigation(
     frame_routing_id = frame_host->GetRoutingID();
   }
   if (!navigation_handle->IsErrorPage()) {
+    // FIXME: All the Emit() calls below could potentially result in |this|
+    // being destroyed (by JS listening for the event and calling
+    // webContents.destroy()).
     auto url = navigation_handle->GetURL();
     bool is_same_document = navigation_handle->IsSameDocument();
     if (is_same_document) {
@@ -1345,6 +1352,9 @@ void WebContents::LoadURL(const GURL& url, const mate::Dictionary& options) {
     params.load_type = content::NavigationController::LOAD_TYPE_DATA;
   }
 
+  // Calling LoadURLWithParams() can trigger JS which destroys |this|.
+  auto weak_this = GetWeakPtr();
+
   params.transition_type = ui::PAGE_TRANSITION_TYPED;
   params.should_clear_history_list = true;
   params.override_user_agent = content::NavigationController::UA_OVERRIDE_TRUE;
@@ -1353,10 +1363,16 @@ void WebContents::LoadURL(const GURL& url, const mate::Dictionary& options) {
   web_contents()->GetController().DiscardNonCommittedEntries();
   web_contents()->GetController().LoadURLWithParams(params);
 
+  // ⚠️WARNING!⚠️
+  // LoadURLWithParams() triggers JS events which can call destroy() on |this|.
+  // It's not safe to assume that |this| points to valid memory at this point.
+  if (!weak_this)
+    return;
+
   // Set the background color of RenderWidgetHostView.
   // We have to call it right after LoadURL because the RenderViewHost is only
   // created after loading a page.
-  auto* const view = web_contents()->GetRenderWidgetHostView();
+  auto* const view = weak_this->web_contents()->GetRenderWidgetHostView();
   if (view) {
     auto* web_preferences = WebContentsPreferences::From(web_contents());
     std::string color_name;

shell/browser/api/event_emitter.h

@@ -107,15 +107,17 @@ class EventEmitter : public Wrappable<T> {
                      v8::Local<v8::Object> event,
                      Args&&... args) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    v8::Locker locker(isolate());
-    v8::HandleScope handle_scope(isolate());
-    EmitEvent(isolate(), GetWrapper(), name, event,
-              std::forward<Args>(args)...);
-    auto context = isolate()->GetCurrentContext();
+    // It's possible that |this| will be deleted by EmitEvent, so save anything
+    // we need from |this| before calling EmitEvent.
+    auto* isolate = this->isolate();
+    v8::Locker locker(isolate);
+    v8::HandleScope handle_scope(isolate);
+    auto context = isolate->GetCurrentContext();
+    EmitEvent(isolate, GetWrapper(), name, event, std::forward<Args>(args)...);
     v8::Local<v8::Value> defaultPrevented;
-    if (event->Get(context, StringToV8(isolate(), "defaultPrevented"))
+    if (event->Get(context, StringToV8(isolate, "defaultPrevented"))
             .ToLocal(&defaultPrevented)) {
-      return defaultPrevented->BooleanValue(isolate());
+      return defaultPrevented->BooleanValue(isolate);
     }
     return false;
   }