Closed Bug 337365 Opened 19 years ago Closed 19 years ago

nsIDocument methods are only usable with MOZILLA_INTERNAL_API defined

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: benjamin)

Details

(Keywords: crash)

Attachments

(1 file)

Simple include fixes, rev. 1
4.70 KB, patch
sicking
: review+
sicking
: superreview+
Details | Diff | Splinter Review
All nsINode functions are in an ifdef MOZILLA_INTERNAL_API. Therefore, if it isn't defined, the compiler uses wrong vtable offsets when calling methods in its subclasses, such as nsIDocument, whose methods are not in a MOZILLA_INTERNAL_API ifdef.
Some possible options: 1. Remove the ifdef from nsINode Bad, as those should be internal 2. Add the right amount of a virtual void fooN(); in a #else Maintenance problem 3. ifdef all of nsIDocument Bad if people actually need to use those 4. Make nsIDocument not inherit from nsINode Bad, misses the point of nsINode 5. Add a new interface nsIPublicDocument (needs a different name). Wouldn't inherit from nsINode. This may be the way to go.
Yeah, i like the nsIPublicDocument solution best. The only issue there is collisions with methods on nsIDocument, but I don't think that'll be a big problem. Do we know what methods people need from nsIDocument? Are some of these methods stable enough that we can put them a public interface that we freeze? Maybe we could even put them at the end of nsIDOMNSDocument.
Why is nsINode ifdefed? Please note that MOZILLA_INTERNAL_API is only about symbol linkage. It shouldn't be overloaded to mean anything about who is allowed to use nonfrozen interface or pseudo-interface APIs.
> Why is nsINode ifdefed? Initially, because nsIContent is and I created nsINode by moving methods from nsIContent to it, figuring that whatever reasoning led to those ifdefs being added to nsIContent still applies.
I'm fine with removing the ifdefs. Hopefully people knows the difference between frozen and unfrozen interfaces, and if they want to depend on a specific version of gecko then that's ok.
Well.. the nsIContent ifdef was added for bug 315562. I'm not sure what that means in terms of nsINode.
I added the ifdefs to nsIContent because that file was #including headers that ended up #including nsString.h, which is only usable with the internal API. Let me try to remove the ifdefs and see whether that causes problems.
If you mean nsINodeInfo.h, nsINode uses that...
There is no reason why nsINodeInfo.h needs to #include any string headers. The only function that seems to be using strings (rather than passing references around) is GetPrefix, which there is no reason not to move to nsNodeInfo.cpp
Flags: blocking1.9a2?
Flags: blocking1.9a2? → blocking1.9a1?
Assignee: general → benjamin
Status: NEW → ASSIGNED
Attachment #230138 - Flags: review?(jst)
Comment on attachment 230138 [details] [diff] [review] Simple include fixes, rev. 1 >Index: content/base/public/nsINodeInfo.h >+#ifdef MOZILLA_INTERNAL_API > /* > * Get the prefix from this node as a string. > * > * For the HTML element "<body>" this will return a null string and for > * the XML element "<html:body>" this will return the string "html". > */ > void GetPrefix(nsAString& aPrefix) const > { > if (mInner.mPrefix) { > mInner.mPrefix->ToString(aPrefix); > } else { > SetDOMStringToNull(aPrefix); > } > } >+#endif Alternativly, simply make this a true virtual function. I would sort of prefer that. Of course, I just added a similar (as in, it uses strings) function to nsIContent where we don't want to make things virtual so...
"using strings" isn't the problem ;-)... only using null strings isn't available from the frozen API.
Ah, so it's the SetDOMStringToNull(aPrefix) call that is the problem here? If so, just replace that with aPrefix.SetIsVoid(PR_True) which I assume is frozen.
No, it's not. The frozen string API does not expose "void" strings at all.
Attachment #230138 - Flags: review?(jst) → review?(bugmail)
Comment on attachment 230138 [details] [diff] [review] Simple include fixes, rev. 1 r/sr=sicking
Attachment #230138 - Flags: superreview+
Attachment #230138 - Flags: review?(bugmail)
Attachment #230138 - Flags: review+
I suspect this has caused the nye bloat tinderbox to catch fire, see http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1154034360.12559.gz&fulltext=1
Yep. nsIDocument::nsIDocument is still MOZILLA_INTERNAL_API, hence the bustage.
I think I fixed the bustage with another #ifdef
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Flags: blocking1.9a1?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: