mirror of
				https://github.com/RGBCube/serenity
				synced 2025-10-31 11:12:45 +00:00 
			
		
		
		
	LibJS+LibWeb: Make HTML::Script GC-allocated
This allows the garbage collector to keep HTML::Script objects alive and fixes a bug where a HTMLScriptElement could get GC'd while its code was executing.
This commit is contained in:
		
							parent
							
								
									00c8f07192
								
							
						
					
					
						commit
						8f9ed415a0
					
				
					 6 changed files with 35 additions and 12 deletions
				
			
		|  | @ -22,6 +22,8 @@ class Script final : public Cell { | ||||||
| public: | public: | ||||||
|     struct HostDefined { |     struct HostDefined { | ||||||
|         virtual ~HostDefined() = default; |         virtual ~HostDefined() = default; | ||||||
|  | 
 | ||||||
|  |         virtual void visit_host_defined_self(Cell::Visitor&) = 0; | ||||||
|     }; |     }; | ||||||
| 
 | 
 | ||||||
|     virtual ~Script() override; |     virtual ~Script() override; | ||||||
|  |  | ||||||
|  | @ -30,6 +30,7 @@ HTMLScriptElement::~HTMLScriptElement() = default; | ||||||
| void HTMLScriptElement::visit_edges(Cell::Visitor& visitor) | void HTMLScriptElement::visit_edges(Cell::Visitor& visitor) | ||||||
| { | { | ||||||
|     Base::visit_edges(visitor); |     Base::visit_edges(visitor); | ||||||
|  |     visitor.visit(m_script); | ||||||
|     visitor.visit(m_parser_document.ptr()); |     visitor.visit(m_parser_document.ptr()); | ||||||
|     visitor.visit(m_preparation_time_document.ptr()); |     visitor.visit(m_preparation_time_document.ptr()); | ||||||
| } | } | ||||||
|  |  | ||||||
|  | @ -77,7 +77,7 @@ private: | ||||||
| 
 | 
 | ||||||
|     Function<void()> m_script_ready_callback; |     Function<void()> m_script_ready_callback; | ||||||
| 
 | 
 | ||||||
|     RefPtr<Script> m_script; |     JS::GCPtr<Script> m_script; | ||||||
| 
 | 
 | ||||||
|     Optional<DOM::DocumentLoadEventDelayer> m_document_load_event_delayer; |     Optional<DOM::DocumentLoadEventDelayer> m_document_load_event_delayer; | ||||||
| 
 | 
 | ||||||
|  |  | ||||||
|  | @ -16,8 +16,10 @@ | ||||||
| namespace Web::HTML { | namespace Web::HTML { | ||||||
| 
 | 
 | ||||||
| // https://html.spec.whatwg.org/multipage/webappapis.html#creating-a-classic-script
 | // https://html.spec.whatwg.org/multipage/webappapis.html#creating-a-classic-script
 | ||||||
| NonnullRefPtr<ClassicScript> ClassicScript::create(String filename, StringView source, EnvironmentSettingsObject& environment_settings_object, AK::URL base_url, size_t source_line_number, MutedErrors muted_errors) | JS::NonnullGCPtr<ClassicScript> ClassicScript::create(String filename, StringView source, EnvironmentSettingsObject& environment_settings_object, AK::URL base_url, size_t source_line_number, MutedErrors muted_errors) | ||||||
| { | { | ||||||
|  |     auto& vm = environment_settings_object.realm().vm(); | ||||||
|  | 
 | ||||||
|     // 1. If muted errors was not provided, let it be false. (NOTE: This is taken care of by the default argument.)
 |     // 1. If muted errors was not provided, let it be false. (NOTE: This is taken care of by the default argument.)
 | ||||||
| 
 | 
 | ||||||
|     // 2. If muted errors is true, then set baseURL to about:blank.
 |     // 2. If muted errors is true, then set baseURL to about:blank.
 | ||||||
|  | @ -29,7 +31,7 @@ NonnullRefPtr<ClassicScript> ClassicScript::create(String filename, StringView s | ||||||
|         source = ""sv; |         source = ""sv; | ||||||
| 
 | 
 | ||||||
|     // 4. Let script be a new classic script that this algorithm will subsequently initialize.
 |     // 4. Let script be a new classic script that this algorithm will subsequently initialize.
 | ||||||
|     auto script = adopt_ref(*new ClassicScript(move(base_url), move(filename), environment_settings_object)); |     auto script = vm.heap().allocate_without_realm<ClassicScript>(move(base_url), move(filename), environment_settings_object); | ||||||
| 
 | 
 | ||||||
|     // 5. Set script's settings object to settings. (NOTE: This was already done when constructing.)
 |     // 5. Set script's settings object to settings. (NOTE: This was already done when constructing.)
 | ||||||
| 
 | 
 | ||||||
|  | @ -45,7 +47,7 @@ NonnullRefPtr<ClassicScript> ClassicScript::create(String filename, StringView s | ||||||
| 
 | 
 | ||||||
|     // 10. Let result be ParseScript(source, settings's Realm, script).
 |     // 10. Let result be ParseScript(source, settings's Realm, script).
 | ||||||
|     auto parse_timer = Core::ElapsedTimer::start_new(); |     auto parse_timer = Core::ElapsedTimer::start_new(); | ||||||
|     auto result = JS::Script::parse(source, environment_settings_object.realm(), script->filename(), script.ptr(), source_line_number); |     auto result = JS::Script::parse(source, environment_settings_object.realm(), script->filename(), script, source_line_number); | ||||||
|     dbgln_if(HTML_SCRIPT_DEBUG, "ClassicScript: Parsed {} in {}ms", script->filename(), parse_timer.elapsed()); |     dbgln_if(HTML_SCRIPT_DEBUG, "ClassicScript: Parsed {} in {}ms", script->filename(), parse_timer.elapsed()); | ||||||
| 
 | 
 | ||||||
|     // 11. If result is a list of errors, then:
 |     // 11. If result is a list of errors, then:
 | ||||||
|  | @ -58,14 +60,14 @@ NonnullRefPtr<ClassicScript> ClassicScript::create(String filename, StringView s | ||||||
|         script->m_error_to_rethrow = parse_error; |         script->m_error_to_rethrow = parse_error; | ||||||
| 
 | 
 | ||||||
|         // 2. Return script.
 |         // 2. Return script.
 | ||||||
|         return script; |         return JS::NonnullGCPtr(*script); | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
|     // 12. Set script's record to result.
 |     // 12. Set script's record to result.
 | ||||||
|     script->m_script_record = result.release_value(); |     script->m_script_record = *result.release_value(); | ||||||
| 
 | 
 | ||||||
|     // 13. Return script.
 |     // 13. Return script.
 | ||||||
|     return script; |     return JS::NonnullGCPtr(*script); | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
| // https://html.spec.whatwg.org/multipage/webappapis.html#run-a-classic-script
 | // https://html.spec.whatwg.org/multipage/webappapis.html#run-a-classic-script
 | ||||||
|  | @ -158,4 +160,15 @@ ClassicScript::ClassicScript(AK::URL base_url, String filename, EnvironmentSetti | ||||||
| 
 | 
 | ||||||
| ClassicScript::~ClassicScript() = default; | ClassicScript::~ClassicScript() = default; | ||||||
| 
 | 
 | ||||||
|  | void ClassicScript::visit_edges(Cell::Visitor& visitor) | ||||||
|  | { | ||||||
|  |     Base::visit_edges(visitor); | ||||||
|  |     visitor.visit(m_script_record); | ||||||
|  | } | ||||||
|  | 
 | ||||||
|  | void ClassicScript::visit_host_defined_self(Cell::Visitor& visitor) | ||||||
|  | { | ||||||
|  |     visitor.visit(this); | ||||||
|  | } | ||||||
|  | 
 | ||||||
| } | } | ||||||
|  |  | ||||||
|  | @ -16,6 +16,8 @@ namespace Web::HTML { | ||||||
| class ClassicScript final | class ClassicScript final | ||||||
|     : public Script |     : public Script | ||||||
|     , public JS::Script::HostDefined { |     , public JS::Script::HostDefined { | ||||||
|  |     JS_CELL(ClassicScript, Script); | ||||||
|  | 
 | ||||||
| public: | public: | ||||||
|     virtual ~ClassicScript() override; |     virtual ~ClassicScript() override; | ||||||
| 
 | 
 | ||||||
|  | @ -23,7 +25,7 @@ public: | ||||||
|         No, |         No, | ||||||
|         Yes, |         Yes, | ||||||
|     }; |     }; | ||||||
|     static NonnullRefPtr<ClassicScript> create(String filename, StringView source, EnvironmentSettingsObject&, AK::URL base_url, size_t source_line_number = 1, MutedErrors = MutedErrors::No); |     static JS::NonnullGCPtr<ClassicScript> create(String filename, StringView source, EnvironmentSettingsObject&, AK::URL base_url, size_t source_line_number = 1, MutedErrors = MutedErrors::No); | ||||||
| 
 | 
 | ||||||
|     JS::Script* script_record() { return m_script_record; } |     JS::Script* script_record() { return m_script_record; } | ||||||
|     JS::Script const* script_record() const { return m_script_record; } |     JS::Script const* script_record() const { return m_script_record; } | ||||||
|  | @ -39,7 +41,10 @@ public: | ||||||
| private: | private: | ||||||
|     ClassicScript(AK::URL base_url, String filename, EnvironmentSettingsObject& environment_settings_object); |     ClassicScript(AK::URL base_url, String filename, EnvironmentSettingsObject& environment_settings_object); | ||||||
| 
 | 
 | ||||||
|     RefPtr<JS::Script> m_script_record; |     virtual void visit_edges(Cell::Visitor&) override; | ||||||
|  |     virtual void visit_host_defined_self(Cell::Visitor&) override; | ||||||
|  | 
 | ||||||
|  |     JS::GCPtr<JS::Script> m_script_record; | ||||||
|     MutedErrors m_muted_errors { MutedErrors::No }; |     MutedErrors m_muted_errors { MutedErrors::No }; | ||||||
|     Optional<JS::Parser::Error> m_error_to_rethrow; |     Optional<JS::Parser::Error> m_error_to_rethrow; | ||||||
| }; | }; | ||||||
|  |  | ||||||
|  | @ -6,16 +6,18 @@ | ||||||
| 
 | 
 | ||||||
| #pragma once | #pragma once | ||||||
| 
 | 
 | ||||||
| #include <AK/RefCounted.h> |  | ||||||
| #include <AK/URL.h> | #include <AK/URL.h> | ||||||
|  | #include <LibJS/Heap/Cell.h> | ||||||
| #include <LibWeb/Forward.h> | #include <LibWeb/Forward.h> | ||||||
| 
 | 
 | ||||||
| namespace Web::HTML { | namespace Web::HTML { | ||||||
| 
 | 
 | ||||||
| // https://html.spec.whatwg.org/multipage/webappapis.html#concept-script
 | // https://html.spec.whatwg.org/multipage/webappapis.html#concept-script
 | ||||||
| class Script : public RefCounted<Script> { | class Script : public JS::Cell { | ||||||
|  |     JS_CELL(Script, JS::Cell); | ||||||
|  | 
 | ||||||
| public: | public: | ||||||
|     virtual ~Script(); |     virtual ~Script() override; | ||||||
| 
 | 
 | ||||||
|     AK::URL const& base_url() const { return m_base_url; } |     AK::URL const& base_url() const { return m_base_url; } | ||||||
|     String const& filename() const { return m_filename; } |     String const& filename() const { return m_filename; } | ||||||
|  |  | ||||||
		Loading…
	
	Add table
		Add a link
		
	
		Reference in a new issue
	
	 Andreas Kling
						Andreas Kling