#1742 Reduce the use of raw shape pointers

Open
opened 1 month ago by Moonchild · 1 comments

Since we keep running into the occasional crashes with the combination of DOM node manipulation, GC and shapes/groups, especially since @g4jc added a ton of complexity for WebComponents, I’m thinking it’d probably be a good idea to replace use of tagged shape pointers with an object that encapsulates the result of a property lookup. Just to give it that extra layer of error handling and GC control. Use of raw pointers in C++ is a notorious pitfall of the language since it itself never does any checking.

Since we keep running into the occasional crashes with the combination of DOM node manipulation, GC and shapes/groups, especially since @g4jc added a ton of complexity for WebComponents, I'm thinking it'd probably be a good idea to replace use of tagged shape pointers with an object that encapsulates the result of a property lookup. Just to give it that extra layer of error handling and GC control. Use of raw pointers in C++ is a notorious pitfall of the language since it itself never does any checking.
Moonchild added this to the 29.1.0 milestone 1 month ago
Moonchild added the
C: Javascript
label 1 month ago
Moonchild added the
Crash
label 1 month ago
Moonchild added the
Enhancement
label 1 month ago
Moonchild self-assigned this 1 month ago
Moonchild commented 1 month ago
Owner

I’ve been trying to port https://bugzilla.mozilla.org/show_bug.cgi?id=1331668 for this but the class definitions used there throw MSVC for a loop.
I posted for help on STack Overflow but after a brief (< 1 day) time of the question being open it was closed by staff for no apparent reason.
So, I can effectively not port this. The code should be valid but SO responses suggest it might not be, while Mozilla still has this code in use, themselves, so I’m totally stumped.

namespace JS {
class PropertyResult
{
...code snipped for brevity...
}
} // namespace JS

namespace js {

template <class Wrapper>
class WrappedPtrOperations<JS::PropertyResult, Wrapper>
{
...code snipped for brevity...
};

template <class Wrapper>
class MutableWrappedPtrOperations<JS::PropertyResult, Wrapper>
  : public WrappedPtrOperations<JS::PropertyResult, Wrapper>
{
...code snipped for brevity...
};

} // namespace js

throws C2988: unrecognizable template declaration/definition on both the template class definitions:
class WrappedPtrOperations<JS::PropertyResult, Wrapper>
class MutableWrappedPtrOperations<JS::PropertyResult, Wrapper>

Is this even valid code? Is it VS2017+ specific/some language extension to C++?

I've been trying to port https://bugzilla.mozilla.org/show_bug.cgi?id=1331668 for this but the class definitions used there throw MSVC for a loop. I posted for help on STack Overflow but after a brief (< 1 day) time of the question being open it was closed by staff for no apparent reason. So, I can effectively not port this. The code should be valid but SO responses suggest it might not be, while Mozilla still has this code in use, themselves, so I'm totally stumped. ```C++ namespace JS { class PropertyResult { ...code snipped for brevity... } } // namespace JS namespace js { template <class Wrapper> class WrappedPtrOperations<JS::PropertyResult, Wrapper> { ...code snipped for brevity... }; template <class Wrapper> class MutableWrappedPtrOperations<JS::PropertyResult, Wrapper> : public WrappedPtrOperations<JS::PropertyResult, Wrapper> { ...code snipped for brevity... }; } // namespace js ``` throws `C2988: unrecognizable template declaration/definition` on both the template class definitions: `class WrappedPtrOperations<JS::PropertyResult, Wrapper>` `class MutableWrappedPtrOperations<JS::PropertyResult, Wrapper>` Is this even valid code? Is it VS2017+ specific/some language extension to C++?
Moonchild removed this from the 29.1.0 milestone 1 month ago
Moonchild removed their assignment 1 month ago
Sign in to join this conversation.
No Milestone
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.