877277 - Move the document.all getter into WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Review assigned to smaug by a fair dice roll.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I think you should nuke the pref and the check for quirks mode, since the spec doesn't have any of that stuff....

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Stack

There's something rotten... nsDocument::Destroy calls nsContentUtils::ReleaseWrapper, which ends up in XPCJSRuntime::RemoveJSHolder. XPCJSRuntime::RemoveJSHolder then tries to assert that there's nothing to trace, but there actually is something to trace: the mAll member. Any hints on how to get around this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Other than moving the "all" object itself to WebIDL?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1.1

Khuey suggested clearing mAll in Destroy; that seems a little less invasive.

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky (:bz) from comment #1)
> I think you should nuke the pref and the check for quirks mode, since the
> spec doesn't have any of that stuff....

+1
@Ms2ger: Please update the quirks-mode-spec if you insist on checking the quirks mode.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 757124 [details] [diff] [review]
Patch v1.1

>@@ -25,9 +25,10 @@ DEPRECATED_OPERATION(DOMExceptionCode)
> DEPRECATED_OPERATION(NoExposedProps)
> DEPRECATED_OPERATION(MutationEvent)
> DEPRECATED_OPERATION(MozSlice)
> DEPRECATED_OPERATION(Components)
> DEPRECATED_OPERATION(PrefixedVisibilityAPI)
> DEPRECATED_OPERATION(NodeIteratorDetach)
> DEPRECATED_OPERATION(MozAudioData)
> DEPRECATED_OPERATION(LenientThis)
>+DEPRECATED_OPERATION(DocumentAll)
I'd prefer to not add this (and just remove the existing warning)

>+nsHTMLDocument::~nsHTMLDocument()
>+{
>+  NS_DROP_JS_OBJECTS(this, nsHTMLDocument);
You should set mAll null before drop, otherwise an assertion may fire.
(The assertion checks that there isn't anything to trace when drop is called.)

>+nsHTMLDocument::ExposeDocumentAll(JSContext* aCx, JSObject* aGlobal)
Could we just not have this stuff.


>+JSObject*
>+nsHTMLDocument::GetAll(JSContext* aCx, ErrorResult& aRv)
>+{
>+  WarnOnceAbout(eDocumentAll);
>+  if (!mAll) {
>+    mAll = JS_NewObject(aCx, &sHTMLDocumentAllClass, nullptr,
>+                        JS_GetGlobalForObject(aCx, GetWrapper()));
>+    if (!mAll) {
>+      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
>+      return nullptr;
>+    }
>+
>+    JS_SetPrivate(mAll, static_cast<nsINode*>(this));
>+    NS_ADDREF_THIS();
Could you copy the comment from the old code.
// Let the JSObject take over ownership of doc.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 757485 [details] [diff] [review]
Patch v2


>+nsHTMLDocument::~nsHTMLDocument()
>+{
>+  NS_DROP_JS_OBJECTS(this, nsHTMLDocument);
Please add mAll = nullptr; before drop.

And crossing fingers :)

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/135a277d1319

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5c81bdff610