560462 - Use fast unwrapping for more quickstubs

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Fast unwrapping, not refcounting return value if possible, return nsContentList so we can get at the nsWrapperCacge.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Part 1 (add helper methods) v1

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Part 2 (use fast unwrapping) v1

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Part 1 (qsgen part) v1

Add support for a general custom quickstub per interface (for example to forward all methods and properties on nsIDOMNSElement to nsGenericElement) without a templated code block.
Add support for methods that return a type that inherits ambiguously from nsISupports (by using a ToSupports function that we can override per type).

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Part 2 (add helper methods) v1

Note that some of these methods have an nsresult outparam, which can only be NS_ERROR_OUT_OF_MEMORY or NS_OK. Maybe I should just remove tha given infallible malloc?

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Part 3 (use fast unwrapping) v1

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 443644 [details] [diff] [review]
Part 2 (add helper methods) v1

- In Element* nsDocument::GetElementById(const nsAString& aElementId, nsresult *aResult):

+  return entry-> GetIdElement();

Remove the space after ->?

- In NS_IMETHODIMP nsDocument::GetElementById(const nsAString& aId, nsIDOMElement** aReturn):

+{
+

Remove the empty line at the beginning of the method?

- In nsHTMLDocument.h:

+  already_AddRefed<nsContentList> GetElementsByName(const nsAString & aName)
+  {
+    nsString* elementNameData = new nsString(aName);
+
+    return elementNameData ?

new is now infallible, no need to null check here.

r=jst

Comment 7

[Johnny Stenback (:jst)]

(In reply to comment #4)
> Created an attachment (id=443644) [details]
> Part 2 (add helper methods) v1
> 
> Note that some of these methods have an nsresult outparam, which can only be
> NS_ERROR_OUT_OF_MEMORY or NS_OK. Maybe I should just remove tha given
> infallible malloc?

It's unclear to me whether or not we can do that yet as malloc() itself is not infallible yet. new and new[] is, bug malloc() is not. If you know the code that's called in those cases uses new and not malloc(), then sure, remove it.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 443666 [details] [diff] [review]
Part 3 (use fast unwrapping) v1

- In nsDOMClassInfoID.h:

+#define DOMCI_CASTABLE_INTERFACE(_interface, _bit, _extra)                    \
+  DOMCI_CASTABLE_INTERFACE(_interface, _interface, _bit, _extra)

This doesn't look like what you want here. And this causes tons of compile time warnings about redefinition of the macro...

The changes in layout/style/nsStyleSet.h look unrelated to these changes?

r=jst

Comment 9

[Johnny Stenback (:jst)]

For the record, this landed as:

http://hg.mozilla.org/mozilla-central/rev/615a41c22413
http://hg.mozilla.org/mozilla-central/rev/e2c5f6be2148
http://hg.mozilla.org/mozilla-central/rev/29558b117b01

with a orange fix attempt as well:

http://hg.mozilla.org/mozilla-central/rev/f30df2c0e098

and then got backed out due to remaining intermittent oranges:

http://hg.mozilla.org/mozilla-central/rev/d67ddb92a98c

The belief is that the intermittent orange was triggered by the "Part 3" patch, and parts 1 and 2 should be able to land independently (which would confirm that belief).

Comment 10

[Johnny Stenback (:jst)]

First two parts (with relevant parts of the orange fix included) landed again as:

http://hg.mozilla.org/mozilla-central/rev/5d1eab36de80
http://hg.mozilla.org/mozilla-central/rev/d1dd21e11f2b

Comment 11

[Peter Van der Beken [:peterv]]

I'm going to land the third part in pieces.

Comment 12

[Peter Van der Beken [:peterv]]

These pieces have landed:

http://hg.mozilla.org/mozilla-central/rev/5005d2023e66
http://hg.mozilla.org/mozilla-central/rev/a206d1143cc2
http://hg.mozilla.org/mozilla-central/rev/5064b94cc8a6
http://hg.mozilla.org/mozilla-central/rev/3ba8b9a5e8a3
http://hg.mozilla.org/mozilla-central/rev/7c3924df9f92
http://hg.mozilla.org/mozilla-central/rev/ad03b2723391
http://hg.mozilla.org/mozilla-central/rev/01a82f7a4e70
http://hg.mozilla.org/mozilla-central/rev/c54e70604905
http://hg.mozilla.org/mozilla-central/rev/38ee63fd5e34
http://hg.mozilla.org/mozilla-central/rev/593d13102b68

One piece had to be backed out because it causes the intermittent failure in a reftest (layout/reftests/bugs/530686-1.html):
http://hg.mozilla.org/mozilla-central/rev/2afbac619d82

After a lot of painful debugging using tryserver I think I know what's going wrong. The problem is only for named DOMCI (NS_DEFINE_CLASSINFO_DATA_WITH_NAME), which means ContentList and MathMLElement.. In XPConnect we store the interfacesbitmap in the XPCNativeScriptableShared and the XPCNativeScriptableShareds are stored in a hashtable. Currently the hashing uses the name of the CI and the flags, but not the interfacesbitmap. So we were sometimes getting a XPCNativeScriptableShared with the right name and flags, but the wrong interfacesbitmap. I think the problem only happens for ContentList, because it uses HTMLCollection as class name but it has a different interfacesbitmap than the HTMLCollection DOCMI. As to why the failure is intermittent, my theory is that it depends on GC timing and whether the conflicting XPCNativeScriptableShared for HTMLCollection has been removed from the hash or not.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Part 4 (fix XPCNativeScriptableShared hashing) v1

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 451044 [details] [diff] [review]
Part 4 (fix XPCNativeScriptableShared hashing) v1

Wow, this does seem like something that would've been "fun" to find!

Comment 15

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/1fe09d6f34d8
http://hg.mozilla.org/mozilla-central/rev/b4504328dd95