Opened 4 years ago

Closed 4 years ago

#16497 closed defect (fixed)

debug build failure in nsContentUtils.cpp

Reported by: mcs Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords:
Cc: arthuredelstein, brade, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I tried to create a debug build using the 5.0a3 Tor Browser code from tor-browser-38.1.0esr-5.0-1 and encountered a compile error in nsContentUtils.cpp. A patch is attached.

Child Tickets

Attachments (1)

nsContentUtils-patch.txt (1.1 KB) - added by mcs 4 years ago.
a possible fix

Download all attachments as: .zip

Change History (6)

Changed 4 years ago by mcs

Attachment: nsContentUtils-patch.txt added

a possible fix

comment:1 Changed 4 years ago by gk

Cc: gk added
Status: newneeds_information

What is causing this given that Mozilla does not need to remove this to have debug builds? What options did you choose for the debug build? Could you add the compile error for posterity?

comment:2 Changed 4 years ago by mcs

The compilation error (from clang) is:

tor-browser/dom/base/nsContentUtils.cpp:3000:19: error: 
      use of undeclared identifier 'aLoadingDocument'; did you mean
      'mozilla::LoadInfo::GetLoadingDocument'?
  NS_PRECONDITION(aLoadingDocument, "Must have a document");
                  ^~~~~~~~~~~~~~~~
                  mozilla::LoadInfo::GetLoadingDocument
../../dist/include/nsDebug.h:93:49: note: expanded from macro 'NS_PRECONDITION'
#define NS_PRECONDITION(expr, str) NS_ASSERTION(expr, str)
                                                ^
../../dist/include/nsDebug.h:82:11: note: expanded from macro 'NS_ASSERTION'
    if (!(expr)) {                                            \
          ^
../../dist/include/mozilla/LoadInfo.h:33:3: note: 
      'mozilla::LoadInfo::GetLoadingDocument' declared here
  NS_DECL_NSILOADINFO
  ^
../../dist/include/nsILoadInfo.h:137:14: note: expanded from macro
      'NS_DECL_NSILOADINFO'
  NS_IMETHOD GetLoadingDocument(nsIDOMDocument * *aLoadingDocument) override; \
             ^
/Users/brade/dev/tor/tor-browser/dom/base/nsContentUtils.cpp:3000:19: error: 
      call to non-static member function without an object argument
  NS_PRECONDITION(aLoadingDocument, "Must have a document");
                  ^~~~~~~~~~~~~~~~
../../dist/include/nsDebug.h:93:49: note: expanded from macro 'NS_PRECONDITION'
#define NS_PRECONDITION(expr, str) NS_ASSERTION(expr, str)
                                                ^
../../dist/include/nsDebug.h:82:11: note: expanded from macro 'NS_ASSERTION'
    if (!(expr)) {                                            \
          ^
2 errors generated.
make[1]: *** [nsContentUtils.o] Error 1

It is caused by one of our patches: ac7de4b0b756891a3835e738ebe534c44d4855c4 (for #13670). That patch replaces the aLoadingDocument parameter with aLoadingNode. Since it is OK for aLoadingNode to be NULL, I do not think we need or want an NS_ASSERTION() for aLoadingNode.

The bottom line: the patch I attached to this bug is a fixup for the #13670 patch (I am sorry I did not mention that before).

comment:3 Changed 4 years ago by gk

So, to understand this fully: This is no error just happening in a debug build. Rather, this is actually a thing clang does not like (while GCC and mingw-w64 + our cross-compiler don't have an issue with it)? If so, what clang version did you use?

comment:4 in reply to:  3 ; Changed 4 years ago by mcs

Status: needs_informationneeds_review

Replying to gk:

So, to understand this fully: This is no error just happening in a debug build. Rather, this is actually a thing clang does not like (while GCC and mingw-w64 + our cross-compiler don't have an issue with it)?

No, I am 99% sure this is a problem that affects all debug builds. I only mentioned clang because the error text that it generates are different than those that gcc generates (never mind). Let me try to explain more clearly.

If you look at the #13670 patch, here:
https://gitweb.torproject.org/tor-browser.git/diff/dom/base/nsContentUtils.cpp?h=tor-browser-38.1.0esr-5.0-1&id=ac7de4b0b756891a3835e738ebe534c44d4855c4
you will see that Arthur removed the aLoadingDocument parameter from nsContentUtils::LoadImage() but he neglected to remove the NS_PRECONDITION() that references it. Does this fix make sense now?

Also, I pushed this fix to the brade tor-browser repo to make it easier to review and merge:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16497-01&id=bfdfcccae99d1af38b3320eb7409377bd0ebc154

comment:5 in reply to:  4 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

If you look at the #13670 patch, here:
https://gitweb.torproject.org/tor-browser.git/diff/dom/base/nsContentUtils.cpp?h=tor-browser-38.1.0esr-5.0-1&id=ac7de4b0b756891a3835e738ebe534c44d4855c4
you will see that Arthur removed the aLoadingDocument parameter from nsContentUtils::LoadImage() but he neglected to remove the NS_PRECONDITION() that references it. Does this fix make sense now?

Yes, thanks.

Also, I pushed this fix to the brade tor-browser repo to make it easier to review and merge:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16497-01&id=bfdfcccae99d1af38b3320eb7409377bd0ebc154

Pushed to tor-browser-38.1.0esr-5.0-1.

Note: See TracTickets for help on using tickets.