The default bug view has changed. See this FAQ.

Use fast unwrapping for more quickstubs

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Fast unwrapping, not refcounting return value if possible, return nsContentList so we can get at the nsWrapperCacge.
(Assignee)

Updated

7 years ago
Depends on: 560273
(Assignee)

Updated

7 years ago
Blocks: 558145
(Assignee)

Comment 1

7 years ago
Created attachment 440796 [details] [diff] [review]
Part 1 (add helper methods) v1
(Assignee)

Comment 2

7 years ago
Created attachment 440797 [details] [diff] [review]
Part 2 (use fast unwrapping) v1
(Assignee)

Updated

7 years ago
Summary: Make GetElement* quickstubs faster → Use fast unwrapping for more quickstubs
(Assignee)

Comment 3

7 years ago
Created attachment 443635 [details] [diff] [review]
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).
Attachment #440796 - Attachment is obsolete: true
Attachment #440797 - Attachment is obsolete: true
Attachment #443635 - Flags: review?(jst)
(Assignee)

Comment 4

7 years ago
Created attachment 443644 [details] [diff] [review]
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?
Attachment #443644 - Flags: review?(jst)
(Assignee)

Comment 5

7 years ago
Created attachment 443666 [details] [diff] [review]
Part 3 (use fast unwrapping) v1
Attachment #443666 - Flags: review?(jst)

Updated

7 years ago
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+
Blocks: 564569
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
(Assignee)

Comment 11

7 years ago
I'm going to land the third part in pieces.
Depends on: 570131
No longer depends on: 570131
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
Created attachment 451044 [details] [diff] [review]
Part 4 (fix XPCNativeScriptableShared hashing) v1
Attachment #451044 - Flags: review?(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!
Attachment #451044 - Flags: review?(jst) → review+
(Assignee)

Comment 15

7 years ago
http://hg.mozilla.org/mozilla-central/rev/1fe09d6f34d8
http://hg.mozilla.org/mozilla-central/rev/b4504328dd95
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 571981
You need to log in before you can comment on or make changes to this bug.