Closed Bug 877277 Opened 7 years ago Closed 7 years ago

Move the document.all getter into WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
Review assigned to smaug by a fair dice roll.
Attachment #755513 - Flags: review?(bugs)
I think you should nuke the pref and the check for quirks mode, since the spec doesn't have any of that stuff....
Attached file Stack (obsolete) —
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?
Other than moving the "all" object itself to WebIDL?
Attached patch Patch v1.1 (obsolete) — Splinter Review
Khuey suggested clearing mAll in Destroy; that seems a little less invasive.
Attachment #755513 - Attachment is obsolete: true
Attachment #757098 - Attachment is obsolete: true
Attachment #755513 - Flags: review?(bugs)
Attachment #757124 - Flags: review?(bugs)
(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.
See Also: → 718923
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.
Attachment #757124 - Flags: review?(bugs) → review-
Attached patch Patch v2Splinter Review
Attachment #757124 - Attachment is obsolete: true
Attachment #757485 - Flags: review?(bugs)
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 :)
Attachment #757485 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/135a277d1319
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 882164
Depends on: 882997
Depends on: 883037
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.