1585819 - MayNeedToLoadXBLBinding should just check the remaining elements that actually might have XBL bindings.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

That is, <panel>, <textbox>, <arrowscrollbox>.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1585819 - Restrict loading XBL bindings from WrapObject to the little amount of XUL elements that remain with bindings.

People keep shooting themselves in the feet because of this codepath and writing
slow code.

There are just a few elements with bindings left, so just check those.

Also simplify a bit the code. The XUL element + tagname check should be pretty
fast now, and ComputedStyle objects no longer keep weak pointers to pres
contexts and such, so can be safely kept on a RefPtr across an style flush.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7f1f760cd45
Restrict loading XBL bindings from WrapObject to the little amount of XUL elements that remain with bindings. r=bzbarsky

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c7e2e49aa84
Somewhat dubiously appease the hazard analysis. r=bustage

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Steve, I think the push in comment 2 exposes a bug in the hazard analysis (fixed by comment 3, I hope).

The function looks something like this:

JSObject* Foo() {
  Rooted<JSObject*> ret = ....;

  RefPtr<SomethingThatCouldTriggerGCOnDestruction> something = ....;

  return ret;
}

The hazard was complaining about: "unrooted '<returnvalue>' of type 'JSObject*' live across GC call at dom/base/Element.cpp:588".

But as far as I can tell, the Rooted destructor should run after the RefPtr destructor, so it should be safe, shouldn't it?

The code used to have an extra scope which I removed, and then reintroduced in comment 3 to hopefully please the analysis, but it seems like a bug to me...

Comment 5

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/d7f1f760cd45
https://hg.mozilla.org/mozilla-central/rev/0c7e2e49aa84

Comment 6

[maggus.staab]

I guess arrowscrollbox was just removed https://bugzilla.mozilla.org/show_bug.cgi?id=1514926

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to maggus.staab from comment #6)

I guess arrowscrollbox was just removed https://bugzilla.mozilla.org/show_bug.cgi?id=1514926

Yeah, ntim sent a patch to remove it in that bug.

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

The hazard was complaining about: "unrooted '<returnvalue>' of type 'JSObject*' live across GC call at dom/base/Element.cpp:588".
But as far as I can tell, the Rooted destructor should run after the RefPtr destructor, so it should be safe, shouldn't it?

Nope, though it totally looks like that should work and we all start out assuming that it does.

The code used to have an extra scope which I removed, and then reintroduced in comment 3 to hopefully please the analysis, but it seems like a bug to me...

The analysis is correct, and I have observed actual crashes from this.

What happens is that return ret extracts a JSObject* from the Rooted<> and places it on the stack (or puts it in a register). Then ~RefPtr runs a GC. The GC updates the pointer in the Rooted<>, but that's unused at this point so doesn't matter. It does not update the return value that is now sitting in a register or on the stack. Finally, ~Rooted runs and unlinks itself from the list of things to update, but that memory location is irrelevant at this point anyway. Control returns to the caller, and it sees a stale pointer as the return value.

This can be quite a nuisance when using RAII objects whose destructors can GC. We just happen to not use many of those within Spidermonkey, and until recently a bug in the analysis prevented it from noticing that those destructors could GC. Also, most of them can't -- for the most part, they'll only GC if the refcount reaches zero, and most of the time that's not going to happen. (Since usually you're assigning into a RefPtr or nsCOMPtr from something that already has a nonzero refcount, then the destructor will just drop it back to that refcount. Unless some code executes within the RAII scope that removes the original storage location.)

Depending on how much of a nuisance this turns out to be in practice, I may need to come up with more ergonomic ways of handling it. I suppose we could use Maybe<RefPtr<T>> and then reset() it explicitly before returning a value (I think the analysis is smart enough to not complain if you return a nullptr in error cases, so you'd only need it in the successful return path.) But that's anything but ergonomic.

When it works, switching to a MutableHandle<> outparam is a good solution (changing the return type to void, bool, or nsresult). But that's not always convenient, especially for heavily-used or idl-generated method signatures.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Huh, TIL. Thanks for taking a look, and sorry for disturbing then :)