1899172 - Remove ScriptFetchOptions::mElement field

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Tooru Fujisawa [:arai]]

ScriptFetchOptions::mElement holds a reference to script element which triggered the fetching.

https://searchfox.org/mozilla-central/rev/3609b689a2c2ba7d75c4f047aee1546b86bc51bb/js/loader/ScriptFetchOptions.h#48,91-98

class ScriptFetchOptions {
...
  /*
   *  Represents fields populated by DOM elements (nonce, parser metadata)
   *  Leave this field as a nullptr for any fetch that requires the
   *  default classic script options.
   *  (https://html.spec.whatwg.org/multipage/webappapis.html#default-classic-script-fetch-options)
   *  TODO: extract necessary fields rather than passing this object
   */
  nsCOMPtr<mozilla::dom::Element> mElement;

the option is held by LoadedScript, which means caching LoadedScript holds the reference to the original script unnecessarily longer.

https://searchfox.org/mozilla-central/rev/3609b689a2c2ba7d75c4f047aee1546b86bc51bb/js/loader/LoadedScript.h#54,57

class LoadedScript : public nsIMemoryReporter {
...
  RefPtr<ScriptFetchOptions> mFetchOptions;

As the comment above mElement suggests, we should copy necessary info into ScriptFetchOptions, so that the reference can be removed.

Comment 1

[Tooru Fujisawa [:arai]]

Actually, there's no longer direct dependency to the script element.
only one case I can think of is the hint charset attribute, but preload handles it separately, and it's also not part of the script fetch options per spec, so we can just move the element field to ScriptLoadContext or somewhere.

anyway, I have some refactoring patch stack that was necessary to figure out the above, and I'll post them here.

Comment 2

[Jon Coppeard (:jonco)]

Thanks for working on this arai. It would be nice to get rid of that element pointer altogether.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 0: Disable bytecode encoding test. r?nbp!

TRACE_FOR_TEST depends on script element reference for imported modules.
This patch stack will remove the reference except for top-level scripts.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 1: Cleanup TRACE_FOR_TEST macros. r?nbp!

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 2: Add ScriptLoadContext::GetInlineScriptText. r?nbp!

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 3: Add ScriptLoadContext::GetHintCharset. r?nbp!

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 4: Add ScriptLoadContext::{GetScriptLineNumber,GetScriptColumnNumber}. r?nbp!

They'll be reworked in later patch not to depend on script element.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 5: Add ScriptLoadContext::HasScriptElement. r?nbp!

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 6: Consistently use ScriptLoadContext::GetParserCreated. r?nbp!

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 7: Add ScriptLoadContext::{BeginEvaluatingTopLevel,EndEvaluatingTopLevel}. r?nbp!

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 8: Add ScriptLoadContext::{UnblockParser,ContinueParserAsync}. r?nbp!

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 9: Add ScriptLoadContext::GetScriptOwnerDocument. r?nbp!

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 10: Add dedicate ScriptLoadContext::GetScriptElement* methods for each purpose. r?nbp!

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1899172 - Part 11: Move nsIScriptElement from ScriptFetchOptions to ScriptLoadContext. r?nbp!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bc1862524f4
Part 0: Disable bytecode encoding test. r=nbp
https://hg.mozilla.org/integration/autoland/rev/28dcd53fa44f
Part 1: Cleanup TRACE_FOR_TEST macros. r=nbp
https://hg.mozilla.org/integration/autoland/rev/8efea89fc137
Part 2: Add ScriptLoadContext::GetInlineScriptText. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7a085fc3c619
Part 3: Add ScriptLoadContext::GetHintCharset. r=nbp
https://hg.mozilla.org/integration/autoland/rev/291409abc485
Part 4: Add ScriptLoadContext::{GetScriptLineNumber,GetScriptColumnNumber}. r=nbp
https://hg.mozilla.org/integration/autoland/rev/ade4594e901a
Part 5: Add ScriptLoadContext::HasScriptElement. r=nbp
https://hg.mozilla.org/integration/autoland/rev/27b7be9780a1
Part 6: Consistently use ScriptLoadContext::GetParserCreated. r=nbp
https://hg.mozilla.org/integration/autoland/rev/e759f417c8bc
Part 7: Add ScriptLoadContext::{BeginEvaluatingTopLevel,EndEvaluatingTopLevel}. r=nbp
https://hg.mozilla.org/integration/autoland/rev/034a04c83391
Part 8: Add ScriptLoadContext::{UnblockParser,ContinueParserAsync}. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7869dd324684
Part 9: Add ScriptLoadContext::GetScriptOwnerDocument. r=nbp
https://hg.mozilla.org/integration/autoland/rev/bf36733567ec
Part 10: Add dedicate ScriptLoadContext::GetScriptElement* methods for each purpose. r=nbp
https://hg.mozilla.org/integration/autoland/rev/fb1ed32eb195
Part 11: Move nsIScriptElement from ScriptFetchOptions to ScriptLoadContext. r=nbp

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/7bc1862524f4
https://hg.mozilla.org/mozilla-central/rev/28dcd53fa44f
https://hg.mozilla.org/mozilla-central/rev/8efea89fc137
https://hg.mozilla.org/mozilla-central/rev/7a085fc3c619
https://hg.mozilla.org/mozilla-central/rev/291409abc485
https://hg.mozilla.org/mozilla-central/rev/ade4594e901a
https://hg.mozilla.org/mozilla-central/rev/27b7be9780a1
https://hg.mozilla.org/mozilla-central/rev/e759f417c8bc
https://hg.mozilla.org/mozilla-central/rev/034a04c83391
https://hg.mozilla.org/mozilla-central/rev/7869dd324684
https://hg.mozilla.org/mozilla-central/rev/bf36733567ec
https://hg.mozilla.org/mozilla-central/rev/fb1ed32eb195