Closed Bug 560462 Opened 10 years ago Closed 10 years ago

Use fast unwrapping for more quickstubs

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: peterv, Assigned: peterv)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

Fast unwrapping, not refcounting return value if possible, return nsContentList so we can get at the nsWrapperCacge.
Depends on: 560273
Blocks: domperf
Attached patch Part 1 (add helper methods) v1 (obsolete) — Splinter Review
Attached patch Part 2 (use fast unwrapping) v1 (obsolete) — Splinter Review
Summary: Make GetElement* quickstubs faster → Use fast unwrapping for more quickstubs
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).
Attachment #440796 - Attachment is obsolete: true
Attachment #440797 - Attachment is obsolete: true
Attachment #443635 - Flags: review?(jst)
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?
Attachment #443644 - Flags: review?(jst)
Attachment #443666 - Flags: review?(jst)
Attachment #443635 - Flags: review?(jst) → review+
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
Attachment #443644 - Flags: review?(jst) → review+
(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 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
Attachment #443666 - Flags: review?(jst) → review+
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).
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
I'm going to land the third part in pieces.
No longer depends on: 570131
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 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!
Attachment #451044 - Flags: review?(jst) → review+
http://hg.mozilla.org/mozilla-central/rev/1fe09d6f34d8
http://hg.mozilla.org/mozilla-central/rev/b4504328dd95
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.