342164 - Permit application to decide when nsExpatDriver writes to the console

Status: RESOLVED FIXED

(Core :: XML, enhancement)

Description

[Robert Sayre]

This needs to be moved to the XML sink or made configurable.

Comment 1

[Alex Vincent [:WeirdAl]]

Reference http://groups.google.com/group/mozilla.dev.tech.xml/browse_thread/thread/c4c8cc9d727b8137/159971fd292faa7b#159971fd292faa7b for a discussion on this.

Comment 2

[Alex Vincent [:WeirdAl]]

Hm.  nsExpatDriver inherits from nsITokenizer and nsIDTD, which are both .h files (not .idl), so they probably can't be reached via script.  I'm fairly sure we wouldn't want to, anyway.

So we need some kind of scriptable interface.

Option 1 would be to add a new interface to nsExpatDriver.

Option 2 would be to modify an existing interface for nsExpatDriver or one of its supporting players.

At present, I prefer modifying nsIExpatSink.idl, which already has a ReportError() method, to include an "ignoreErrorConsole" boolean value.  This value defaults to false.  Several content sinks implement this interface.  When this value is true, the expat driver can then ignore it.

Alternatively, I could add it on nsIExtendedExpatSink.idl, which only nsSAXXMLReader implements.

Thoughts?

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

I've tested this, and it works.

Comment 4

[Robert Sayre]

Please take the switch out of SAX. Consumers get a reporterror event, so they can log it if they want to.

Comment 5

[Alex Vincent [:WeirdAl]]

Comment on attachment 227603 [details] [diff] [review]
patch, v1

What sort of event?  It can't be a DOM event, because there's literally no DOM nodes to dispatch it to.

Comment 6

[Alex Vincent [:WeirdAl]]

Besides, the console logging has taken place before ReportError gets called:
http://lxr.mozilla.org/seamonkey/source/parser/htmlparser/src/nsExpatDriver.cpp#913

So I really don't understand what you mean.

Comment 7

[Robert Sayre]

(In reply to comment #6)
> Besides, the console logging has taken place before ReportError gets called:
> 

Yeah, that's bad IMHO. SAX users have an error handler that handles error events (event in the general sense). They can log it if they want to.

Comment 8

[Alex Vincent [:WeirdAl]]

I'm doing some thinking, and I don't know if I like what I think.

What you're suggesting means moving the error reporting out of nsExpatDriver and placing it into whoever implements nsISAXErrorHandler.  At the very least to maintain current functionality, this means API changes to nsIExpatSink's reportError, and then each implementation has to decide on its own whether or not to log the error to the console.

In the case of nsXMLSAXReader, passing it onto the error handler would mean constructing the locator object from the new set of arguments.

On top of that, we end up with a fair bit of code duplication in the other content sinks, which each must continue to report the XML errors we currently get.

The reason I don't like what I'm thinking isn't because it's hard.  It's actually pretty easy.  It's because we would want to try doing the same for 1.8 branch as well, and we are running out of time for API changes like this.  The alternative is a known API change between 1.8 branch and trunk that might have been avoidable.

Robert, if you agree, please request blocking1.8.1.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2

This is my best guess as to the right solution.  It's a little bit bigger, but largely preserves current behavior.  What this patch changes that doesn't remain current behavior:

* Not-well-formed overlays don't get a second message logged to the console (bug 342063)
* SAXXMLReaders defer error reporting to the error handler if there is one, and if there isn't one, it doesn't report the error.
* SAXXMLReaders create SAXLocator objects now for error handlers.  (This is why I added a ReportScriptError method to nsIExpatSink.)

I wasn't sure if I should adopt the various ReportError implementations into ReportScriptError, but I felt the ReportError code should be available as a fallback for ReportScriptError.  In addition, ReportScriptError can call on ReportError just as easily.

One thing I haven't figured out yet:  the line numbers tend to be a bit inconsistent, especially when I'm typing in an undefined entity or an element tag without a node name yet ("</", or "<").  But that's a different bug.

Comment 10

[Alex Vincent [:WeirdAl]]

Comment on attachment 227757 [details] [diff] [review]
patch, v2

>+  nsSAXLocator* locator = new nsSAXLocator;
...
>   if (mErrorHandler) {
>-    return mErrorHandler->FatalError(nsnull, nsDependentString(aErrorText));
>+    return mErrorHandler->FatalError(locator, nsDependentString(aErrorText));
>   }

<biesi> you leak the saxlocator
<biesi> you should put it into a comptr or refptr

Oops.  I can also optimize the function a bit by returning earlier for !mErrorHandler.

Comment 11

[Peter Van der Beken [:peterv]]

There's no reason to have two methods to report an error on that interface.

Comment 12

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3

Per discussion with peterv, I've postponed the SAXLocator code for another bug.

Comment 13

[Alex Vincent [:WeirdAl]]

Comment on attachment 228340 [details] [diff] [review]
patch, v3

wrong patch (grr)

Comment 14

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3

Let's try that again.

Comment 15

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3.1

Per discussion with sicking and sayrer, we've decided to report errors by default in the SAX Reader's ReportError.  If the reader's consumer set an error handler, we don't report the error; we let the error handler determine what to do.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 229529 [details] [diff] [review]
patch, v3.1

>Index: parser/htmlparser/public/nsIExpatSink.idl

> #include "nsISupports.idl"
>+#include "nsIScriptError.idl"

You don't need to #include nsIScriptError.idl here. Just forward declare the interface. That way you won't have to add the REQUIRES either.


>Index: parser/xml/src/nsSAXXMLReader.cpp

> nsSAXXMLReader::ReportError(const PRUnichar* aErrorText,
>                             const PRUnichar* aSourceText,
>-                            PRInt32 aLineNumber,
>-                            PRInt32 aColumnNumber)
>+                            nsIScriptError* aError)
> {
>   /// XXX need to settle what to do about the input setup, so I have
>   /// coherent values for the nsISAXLocator here. nsnull for now.
>   if (mErrorHandler)
>     return mErrorHandler->FatalError(nsnull, nsDependentString(aErrorText));
>+  else {

Please put { } on the |if| part here as well.

>+    nsCOMPtr<nsIConsoleService> cs
>+     (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+    if (cs)
>+    {

Follow convention and put the '{' on the same line as the |if|


>Index: rdf/base/src/nsRDFContentSink.cpp

> RDFContentSinkImpl::ReportError(const PRUnichar* aErrorText, 
>                                 const PRUnichar* aSourceText,
>-                                PRInt32 aLineNumber,
>-                                PRInt32 aColumnNumber)
>+                                nsIScriptError* aError)
> {
>-  return NS_OK;
>+  NS_PRECONDITION(aError && aSourceText && aErrorText, "Check arguments!!!");
>+  nsCOMPtr<nsIConsoleService> cs
>+     (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+  nsresult rv = NS_OK;
>+  if (cs)
>+  {

Same here


>Index: content/xml/document/src/nsXMLContentSink.cpp
> nsXMLContentSink::ReportError(const PRUnichar* aErrorText, 
>                               const PRUnichar* aSourceText,
>-                              PRInt32 aLineNumber,
>-                              PRInt32 aColumnNumber)
>+                              nsIScriptError *aError)
> {
>-  nsresult rv = NS_OK;
>-  
>+  NS_PRECONDITION(aError && aSourceText && aErrorText, "Check arguments!!!");
>+  nsresult rv;
>+  nsCOMPtr<nsIConsoleService> cs
>+     (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+  if (cs)
>+  {

And here

>Index: content/xml/document/src/nsXMLFragmentContentSink.cpp

> nsXMLFragmentContentSink::ReportError(const PRUnichar* aErrorText, 
>                                       const PRUnichar* aSourceText,
>-                                      PRInt32 aLineNumber,
>-                                      PRInt32 aColumnNumber)
>+                                      nsIScriptError *aError)
> {
>   mParseError = PR_TRUE;
>-  // The following error reporting is copied from nsXBLContentSink::ReportError()
> 
>-  nsCOMPtr<nsIConsoleService> consoleService =
>-    do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>-  if (consoleService) {
>-    consoleService->LogStringMessage(aErrorText);
>+  NS_PRECONDITION(aError && aSourceText && aErrorText, "Check arguments!!!");
>+  nsCOMPtr<nsIConsoleService> cs
>+     (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+  nsresult rv;
>+  if (cs)
>+  {

And here


>Index: content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp
>Index: content/xul/document/src/nsXULContentSink.cpp

and in these files.

Also, you removed the initialization of the |rv| variable in a bunch of places. Did you make sure that that didn't introduce any uses of it uninitialized?

r=sicking with that fixed.

Comment 17

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3.1 with r+ nits answered

Thanks for the review, sicking - it's better to initialize rv than not anyway.  I had some nasty bustages from not initializing a variable on Linux once (xpathgen)...

Also, I had a few compile failures on this pass, mandating I add #include nsIScriptError.h a couple more times.

Comment 18

[Alex Vincent [:WeirdAl]]

Attachment: chrome testcase

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 231918 [details] [diff] [review]
patch, v3.1 with r+ nits answered

>Index: parser/htmlparser/public/nsIExpatSink.idl
>===================================================================
>RCS file: /cvsroot/mozilla/parser/htmlparser/public/nsIExpatSink.idl,v
>retrieving revision 1.10
>diff -p -u -8 -r1.10 nsIExpatSink.idl
>--- parser/htmlparser/public/nsIExpatSink.idl	21 Jan 2006 02:02:01 -0000	1.10
>+++ parser/htmlparser/public/nsIExpatSink.idl	3 Aug 2006 06:44:02 -0000
>@@ -31,24 +31,25 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
>+interface nsIScriptError;
> 
> /**
>  * This interface should be implemented by any content sink that wants
>  * to get output from expat and do something with it; in other words,
>  * by any sink that handles some sort of XML dialect.
>  */
> 
>-[scriptable, uuid(34CB3B34-7F15-4971-B262-370CE61ADE4D)]
>+[scriptable, uuid(f61c56b5-ea5b-42b4-ad3c-17416e72e238)]
> interface nsIExpatSink : nsISupports 
> {
>   /**
>    * Called to handle the opening tag of an element.
>    * @param aName the fully qualified tagname of the element
>    * @param aAtts the array of attribute names and values.  There are
>    *        aAttsCount/2 names and aAttsCount/2 values, so the total number of
>    *        elements in the array is aAttsCount.  The names and values
>@@ -124,13 +125,19 @@ interface nsIExpatSink : nsISupports 
>    * @param aStandalone -1, 0, or 1 indicating respectively that there was no
>    *                    standalone parameter in the declaration, that it was
>    *                    given as no, or that it was given as yes.
>    */
>   void HandleXMLDeclaration(in wstring aVersion,
>                             in wstring aEncoding,
>                             in long aStandalone);
> 
>+  /**
>+   * Report an error to the content sink using a nsIScriptError.
>+   *
>+   * @param aErrorText  Error message to pass to content sink.
>+   * @param aSourceText Source text of the document we're parsing.
>+   * @param aError      Script error object with line number & column number
>+   */
>   void ReportError(in wstring aErrorText,
>                    in wstring aSourceText,
>-                   in long aLineNumber,
>-                   in long aColumnNumber);
>+                   in nsIScriptError aError);
> }; 
>Index: parser/htmlparser/src/nsExpatDriver.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/parser/htmlparser/src/nsExpatDriver.cpp,v
>retrieving revision 3.100
>diff -p -u -8 -r3.100 nsExpatDriver.cpp
>--- parser/htmlparser/src/nsExpatDriver.cpp	21 Jul 2006 16:28:31 -0000	3.100
>+++ parser/htmlparser/src/nsExpatDriver.cpp	3 Aug 2006 06:44:03 -0000
>@@ -906,37 +906,38 @@ nsExpatDriver::HandleError()
>   // Adjust the column number so that it is one based rather than zero based.
>   PRUint32 colNumber = XML_GetCurrentColumnNumber(mExpatParser) + 1;
>   PRUint32 lineNumber = XML_GetCurrentLineNumber(mExpatParser);
> 
>   nsAutoString errorText;
>   CreateErrorText(description.get(), XML_GetBase(mExpatParser), lineNumber,
>                   colNumber, errorText);
> 
>-  nsCOMPtr<nsIConsoleService> cs
>-    (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>   nsCOMPtr<nsIScriptError> serr(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));
>-  if (serr && cs) {
>-    if (NS_SUCCEEDED(serr->Init(description.get(),
>-                                mURISpec.get(),
>-                                mLastLine.get(),
>-                                lineNumber, colNumber,
>-                                nsIScriptError::errorFlag, "malformed-xml")))
>-      cs->LogMessage(serr);
>+  nsresult rv = NS_OK;
>+  if (serr) {
>+    rv = serr->Init(description.get(),
>+                    mURISpec.get(),
>+                    mLastLine.get(),
>+                    lineNumber, colNumber,
>+                    nsIScriptError::errorFlag, "malformed-xml");
>+  }
>+  if (!serr || NS_FAILED(rv))
>+  {

Inconsistent bracing.

>Index: parser/xml/src/nsSAXXMLReader.cpp
>===================================================================

> nsSAXXMLReader::ReportError(const PRUnichar* aErrorText,
>                             const PRUnichar* aSourceText,
>-                            PRInt32 aLineNumber,
>-                            PRInt32 aColumnNumber)
>+                            nsIScriptError* aError)
> {
>   /// XXX need to settle what to do about the input setup, so I have
>   /// coherent values for the nsISAXLocator here. nsnull for now.
>-  if (mErrorHandler)
>+  if (mErrorHandler) {
>     return mErrorHandler->FatalError(nsnull, nsDependentString(aErrorText));
>+  }
>+ 
>+  nsCOMPtr<nsIConsoleService> cs
>+   (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+  if (cs) {
>+    nsresult rv = cs->LogMessage(aError);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
> 
>   return NS_OK;

  return cs ? cs->LogMessage(aError) : NS_OK;

>Index: rdf/base/src/nsRDFContentSink.cpp
>===================================================================

> RDFContentSinkImpl::ReportError(const PRUnichar* aErrorText, 
>                                 const PRUnichar* aSourceText,
>-                                PRInt32 aLineNumber,
>-                                PRInt32 aColumnNumber)
>+                                nsIScriptError* aError)
> {
>-  return NS_OK;
>+  NS_PRECONDITION(aError && aSourceText && aErrorText, "Check arguments!!!");
>+  nsCOMPtr<nsIConsoleService> cs
>+     (do_GetService(NS_CONSOLESERVICE_CONTRACTID));
>+  nsresult rv = NS_OK;
>+  if (cs) {
>+    rv = cs->LogMessage(aError);
>+  }
>+
>+  // XXX ajvincent nsExpatDriver doesn't check the return value.
>+  return rv;

  return cs ? cs->LogMessage(aError) : NS_OK;

>Index: content/xbl/src/nsXBLContentSink.cpp
>===================================================================

> nsXBLContentSink::ReportError(const PRUnichar* aErrorText, 
>                               const PRUnichar* aSourceText,
>-                              PRInt32 aLineNumber,
>-                              PRInt32 aColumnNumber)
>+                              nsIScriptError* aError)
> {
>+  // XXX ajvincent Reading over this function, I think it's time for it
>+  // to finally die...

No need for this comment.

>+
>   // XXX We should make sure the binding has no effect, but that it also
>   // gets destroyed properly without leaking.  Perhaps we should even
>   // ensure that the content that was bound is displayed with no
>   // binding.
> 
>   // Report the error to the error console.
>   nsCOMPtr<nsIConsoleService> consoleService =
>     do_GetService(NS_CONSOLESERVICE_CONTRACTID);

Don't you want to remove this?

>Index: content/xml/document/src/nsXMLFragmentContentSink.cpp
>===================================================================

> NS_IMETHODIMP
> nsXMLFragmentContentSink::ReportError(const PRUnichar* aErrorText, 
>                                       const PRUnichar* aSourceText,
>-                                      PRInt32 aLineNumber,
>-                                      PRInt32 aColumnNumber)
>+                                      nsIScriptError *aError)
> {
>   mParseError = PR_TRUE;
>-  // The following error reporting is copied from nsXBLContentSink::ReportError()
> 
>-  nsCOMPtr<nsIConsoleService> consoleService =
>-    do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>-  if (consoleService) {
>-    consoleService->LogStringMessage(aErrorText);
>+  NS_PRECONDITION(aError && aSourceText && aErrorText, "Check arguments!!!");

NS_PRECONDITION should be the first line of a function.

>Index: content/xul/document/src/nsXULContentSink.cpp
>===================================================================

>-  if (doc && !doc->OnDocumentParserError()) {

Can't you remove OnDocumentParserError then?

Comment 20

[Alex Vincent [:WeirdAl]]

peterv: is this a formal super-review?  (If not, that's okay; I just want to know where to stand with my patch.)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 231918 [details] [diff] [review]
patch, v3.1 with r+ nits answered

>Index: parser/htmlparser/src/nsExpatDriver.cpp
>+  if (!serr || NS_FAILED(rv))

Why bother evaluating this in non-debug builds?  Please put an #ifdef DEBUG around this whole block.

>Index: parser/xml/src/nsSAXXMLReader.cpp
>+  if (cs) {
>+    nsresult rv = cs->LogMessage(aError);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
> 
>   return NS_OK;

Why not make this code look like the RDF sink code does?  That would make more sense to me.

>Index: rdf/base/src/nsRDFContentSink.cpp
>+  // XXX ajvincent nsExpatDriver doesn't check the return value.

So?  That seems pretty reasonable to me.  Please remove this comment.

>Index: content/xbl/src/nsXBLContentSink.cpp
>+  // XXX ajvincent Reading over this function, I think it's time for it
>+  // to finally die...

If you really think so, change this to a FIXME comment referencing the bug# filed on the matter.

It does look like the LogStringMessage call here should be removed.  Why did you leave it?

>Index: content/xslt/src/xslt/txMozillaStylesheetCompiler.cpp
>+        nsresult rv = cs->LogMessage(aError);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+
>     mCompiler->cancel(NS_ERROR_FAILURE, aErrorText, aSourceText);

You should call cancel() even if LogMessage() fails.  Similar for all other places where you added early returns; check to make sure you're not skipping necessary cleanup code.

>Index: content/xul/document/src/nsXULContentSink.cpp

The changes to this file are wrong.  You need to keep calling OnDocumentParserError and doing the right thing if it returns PR_FALSE.  I wonder whether you even tested the bug that that code was added for with your patch...

>+  // Give us our "not well formed" yellow screen of significant injury.

What does the last part of this comment mean?

Comment 22

[Boris Zbarsky [:bzbarsky]]

I should note that Peter already pointed out the problem with OnDocumentParserError()...

Comment 23

[Alex Vincent [:WeirdAl]]

(In reply to comment #21)
> >Index: content/xbl/src/nsXBLContentSink.cpp
> >+  // XXX ajvincent Reading over this function, I think it's time for it
> >+  // to finally die...
> 
> If you really think so, change this to a FIXME comment referencing the bug#
> filed on the matter.
> 
> It does look like the LogStringMessage call here should be removed.  Why did
> you leave it?

I prefer being conservative with my patches:  minimal change.  Actually, your two replies on this one function complement each other.  This function overrides nsXMLContentSink::ReportError (and calls on it!).  That's why I left it - as precisely the sort of FIXME issue you mention.

> >Index: content/xul/document/src/nsXULContentSink.cpp
> 
> The changes to this file are wrong.  You need to keep calling
> OnDocumentParserError and doing the right thing if it returns PR_FALSE.  I
> wonder whether you even tested the bug that that code was added for with your
> patch...

Okay, I see what I did wrong.  The intent of the new function was to correctly report well-formedness errors as well.  I'll need to do some additional tests here then to determine how best to handle this.

> >+  // Give us our "not well formed" yellow screen of significant injury.
> 
> What does the last part of this comment mean?

Just a joke referring to the classic Microsoft BSOD.

Comment 24

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3.1 with sr- nits answered

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 232656 [details] [diff] [review]
patch, v3.1 with sr- nits answered

From my previous set of comments:

  "Similar for all other places where you added early returns; check
   to make sure you're not skipping necessary cleanup code."

What happened to that?  There are at least three places (XML, XML fragment, XUL content sinks) where you're skipping necessary cleanup code...

The rest looks fine.

Comment 26

[Alex Vincent [:WeirdAl]]

Attachment: patch, v3.2

I also fixed another issue that I'd been warned about, but didn't see until just now.  nsXBLContentSink was logging the error, and then calling on nsXMLContentSink::ReportError, which of course would log it again.  That's gone now.  Oops.  :)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 235054 [details] [diff] [review]
patch, v3.2

sr=bzbarsky

If you removed the redundant XBL logging here, what's the followup about?

Comment 28

[Alex Vincent [:WeirdAl]]

(In reply to comment #27)
> If you removed the redundant XBL logging here, what's the followup about?

Just actually removing the ReportError function and having XBLContentSink inherit ReportError from XMLContentSink.  Minor code cleanup; the only thing XBLContentSink does that XMLContentSink doesn't do is a couple printf's.

Thanks for the reviews, sicking, bz!

Comment 29

[Boris Zbarsky [:bzbarsky]]

Although, wait.  Now that I think about this a second time...

If we're changing nsIExpatSink anyway, why not make ReportError return a boolean?  If it returns true, nsExpatDriver logs the error.  Otherwise, it doesn't.  then you can remove all that code duplication from all those nsIExpatSink impls.

Comment 30

[Alex Vincent [:WeirdAl]]

Attachment: patch, v4

Based on bz's very smart idea, I've rewritten the patch.

I'm not obsoleting the currently r+/sr+'d patch, just in case.  :)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 235206 [details] [diff] [review]
patch, v4

>Index: parser/htmlparser/public/nsIExpatSink.idl
>+   * @retval True if the content sink doesn't want the error reported.

Please reverse that.  It makes more sense to return false to indicate "don't report an error" from a method called ReportError.

>+  PRBool ReportError(in wstring aErrorText,

s/PRBool/boolean/

>Index: parser/htmlparser/src/nsExpatDriver.cpp
>     mSink->ReportError(errorText.get(), 

You can't ignore the return value here.  The value of didReportError is undefined if ReportError returns failure.

Other than that, looks good.

Comment 32

[Alex Vincent [:WeirdAl]]

Attachment: patch, v4 with sr- nits fixed

I also reworked the logic to account for when serr fails to initialize.

Comment 33

[Alex Vincent [:WeirdAl]]

Attachment: final patch

With fix for last nit, r+/sr+ per bzbarsky on #developers.  Setting r? per bz's request so he can check in the final patch.  Thanks again!

Comment 34

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.