Last Comment Bug 560462 - Use fast unwrapping for more quickstubs
: Use fast unwrapping for more quickstubs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Peter Van der Beken [:peterv]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 560273 571981
Blocks: domperf 564569
  Show dependency treegraph
 
Reported: 2010-04-20 00:24 PDT by Peter Van der Beken [:peterv]
Modified: 2010-06-15 08:49 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 (add helper methods) v1 (15.80 KB, patch)
2010-04-22 09:41 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
Part 2 (use fast unwrapping) v1 (31.30 KB, patch)
2010-04-22 09:42 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
Part 1 (qsgen part) v1 (6.88 KB, patch)
2010-05-05 09:28 PDT, Peter Van der Beken [:peterv]
jst: review+
Details | Diff | Splinter Review
Part 2 (add helper methods) v1 (21.61 KB, patch)
2010-05-05 09:53 PDT, Peter Van der Beken [:peterv]
jst: review+
Details | Diff | Splinter Review
Part 3 (use fast unwrapping) v1 (20.75 KB, patch)
2010-05-05 10:57 PDT, Peter Van der Beken [:peterv]
jst: review+
Details | Diff | Splinter Review
Part 4 (fix XPCNativeScriptableShared hashing) v1 (1.95 KB, patch)
2010-06-14 08:12 PDT, Peter Van der Beken [:peterv]
jst: review+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2010-04-20 00:24:25 PDT
Fast unwrapping, not refcounting return value if possible, return nsContentList so we can get at the nsWrapperCacge.
Comment 1 Peter Van der Beken [:peterv] 2010-04-22 09:41:39 PDT
Created attachment 440796 [details] [diff] [review]
Part 1 (add helper methods) v1
Comment 2 Peter Van der Beken [:peterv] 2010-04-22 09:42:11 PDT
Created attachment 440797 [details] [diff] [review]
Part 2 (use fast unwrapping) v1
Comment 3 Peter Van der Beken [:peterv] 2010-05-05 09:28:17 PDT
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).
Comment 4 Peter Van der Beken [:peterv] 2010-05-05 09:53:43 PDT
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?
Comment 5 Peter Van der Beken [:peterv] 2010-05-05 10:57:10 PDT
Created attachment 443666 [details] [diff] [review]
Part 3 (use fast unwrapping) v1
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2010-05-05 17:16:50 PDT
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, jst@mozilla.com) 2010-05-05 17:44:17 PDT
(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, jst@mozilla.com) 2010-05-05 18:12:12 PDT
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, jst@mozilla.com) 2010-05-14 09:22:46 PDT
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, jst@mozilla.com) 2010-05-18 12:45:00 PDT
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] 2010-06-01 03:55:11 PDT
I'm going to land the third part in pieces.
Comment 12 Peter Van der Beken [:peterv] 2010-06-10 03:39:50 PDT
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] 2010-06-14 08:12:18 PDT
Created attachment 451044 [details] [diff] [review]
Part 4 (fix XPCNativeScriptableShared hashing) v1
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2010-06-14 08:47:14 PDT
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!

Note You need to log in before you can comment on or make changes to this bug.