Stop building the XPCOM glue

RESOLVED FIXED in mozilla53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 6 obsolete attachments)

58 bytes, text/x-review-board-request
dminor
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
glandium
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
Once we don't need the glue for the Firefox stub, we should stop building it. This is currently blocked on bug 1306237.
Priority: -- → P3
Comment on attachment 8809094 [details]
Bug 1306329 part A - nsUTF8Utils is used from outside libxul, and NS_WARNING now only works from within libxul, so make its usage conditional,

https://reviewboard.mozilla.org/r/91742/#review91670

Minor preference to `#undef UTF8UTILS_WARNING` at the end of the header, but I suppose it doesn't matter too much.
Attachment #8809094 - Flags: review?(nfroyd) → review+
Assignee: nobody → benjamin
Comment on attachment 8811001 [details]
Bug 1306329 part B - Remove a few crashreporter tests which can no longer be implemented without linking to internal symbols.

https://reviewboard.mozilla.org/r/93254/#review93238

::: toolkit/crashreporter/test/nsTestCrasher.cpp
(Diff revision 1)
>      PureVirtualCall();
>      // not reached
>      break;
>    }
> -  case CRASH_RUNTIMEABORT: {
> -    NS_RUNTIMEABORT("Intentional crash");

We could probably replace this with just calling `nsIDebug2::abort` from JS, right? I'm not sure if that buys us much over the `MOZ_CRASH` case.
Attachment #8811001 - Flags: review?(ted) → review+
Comment on attachment 8811002 [details]
Bug 1306329 part C - things that depend on xul should no longer link the XPCOM glue library,

https://reviewboard.mozilla.org/r/93256/#review95156

Until comm-central and b2g are ready to not use the xpcom glue, this should be behind some flag (NO_XPCOM_GLUE?)
Attachment #8811002 - Flags: review?(mh+mozilla)
Comment on attachment 8811004 [details]
Bug 1306329 part E - Don't build the dependent XPCOM glue.

https://reviewboard.mozilla.org/r/93260/#review95158

Same comment as part D. xpcom/glue/staticruntime should also receive the same treatment.
Attachment #8811004 - Flags: review?(mh+mozilla)
Comment on attachment 8811005 [details]
Bug 1306329 part F - Stop exporting XPCOM and XUL symbols,

https://reviewboard.mozilla.org/r/93262/#review95160

Same comment as part D and E.
Attachment #8811005 - Flags: review?(mh+mozilla)
Keywords: leave-open
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77f704be0a6d
part A - nsUTF8Utils is used from outside libxul, and NS_WARNING now only works from within libxul, so make its usage conditional, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba830898e8c
part B - Remove a few crashreporter tests which can no longer be implemented without linking to internal symbols. r=ted
Attachment #8809094 - Attachment is obsolete: true
Attachment #8811001 - Attachment is obsolete: true
Attachment #8811002 - Attachment is obsolete: true
Attachment #8811002 - Flags: review?(mh+mozilla)
Attachment #8811003 - Attachment is obsolete: true
Attachment #8811004 - Attachment is obsolete: true
Attachment #8811004 - Flags: review?(mh+mozilla)
Attachment #8811005 - Attachment is obsolete: true
Attachment #8811005 - Flags: review?(mh+mozilla)
I apologize for the spam; reviewboard totally didn't do what I expected when adding a part on the end.
Comment on attachment 8818292 [details]
Bug 1306329 part G - Disable two webrtc tests because they link the XPCOM glue which no longer exists,

https://reviewboard.mozilla.org/r/98406/#review98664
Attachment #8818292 - Flags: review?(dminor) → review+
Duplicate of this bug: 1322707
Should these patches update ALLOWED_XPCOM_GLUE in python/mozbuild/mozbuild/frontend/emitter.py?
(In reply to Nicholas Nethercote [:njn] from comment #28)
> Should these patches update ALLOWED_XPCOM_GLUE in
> python/mozbuild/mozbuild/frontend/emitter.py?

Yes or this will cause a build unit test failure, sorry I missed that in the review.
Blocks: 1322707
Comment on attachment 8818295 [details]
Bug 1306329 part D - Remove unneeded nsXPCOMGlue includes,

https://reviewboard.mozilla.org/r/98410/#review99050

This part is needed *before* bug 1306327, so this can't be in this bug.
Attachment #8818295 - Flags: review+
(In reply to Mike Hommey [:glandium] from comment #31)
> Comment on attachment 8818295 [details]
> Bug 1306329 part D - Remove unneeded nsXPCOMGlue includes,
> 
> https://reviewboard.mozilla.org/r/98410/#review99050
> 
> This part is needed *before* bug 1306327, so this can't be in this bug.

Landed in bug 1329932.
Comment on attachment 8818294 [details]
Bug 1306329 part C - things that depend on xul should no longer link the XPCOM glue library,

https://reviewboard.mozilla.org/r/98408/#review104328

This patch requires the ALLOWED_XPCOM_GLUE stuff in emitter.py to be removed first, otherwise, the build fails during spline reticulation.

::: build/gecko_templates.mozbuild:29
(Diff revision 1)
> -        xpcomglue = 'xpcomglue'
> +        pass
>      elif msvcrt == 'static':
>          USE_STATIC_LIBS = True
> -        xpcomglue = 'xpcomglue_staticruntime'

This should be kept. While we may not have binaries linked to the static CRT at the moment, that might happen at any time, and we need to keep that working.
Attachment #8818294 - Flags: review?(mh+mozilla)
Comment on attachment 8818296 [details]
Bug 1306329 part E - Don't build the dependent XPCOM glue.

https://reviewboard.mozilla.org/r/98412/#review104334

::: xpcom/glue/moz.build:15
(Diff revision 1)
>  DIRS += ['standalone']
>  
>  # On win we build two glue libs - glue linked to crt dlls here and in staticruntime we build
>  # a statically linked glue lib.
>  if CONFIG['OS_ARCH'] == 'WINNT':
>      DIRS += ['staticruntime']

We don't need the static runtime version of the dependent xpcom glue anymore, you can remove this line and the corresponding directory.

::: xpcom/glue/moz.build
(Diff revision 1)
>      'Mutex.h',
>      'Observer.h',
>      'ReentrantMonitor.h',
>  ]
>  
> -include('objs.mozbuild')

After this, nothing uses objs.mozbuild but xpcom/build/moz.build. We should move its contents there (and, in fact, probably move the files too). But that can be done in a followup.

::: xpcom/glue/moz.build:87
(Diff revision 1)
> -]
> -
> -# Force to build a static library only
> -NO_EXPAND_LIBS = True
> -
>  DIST_INSTALL = True

This line can be removed too,
Attachment #8818296 - Flags: review?(mh+mozilla)
Comment on attachment 8818297 [details]
Bug 1306329 part F - Stop exporting XPCOM and XUL symbols,

https://reviewboard.mozilla.org/r/98414/#review104344

Most changes under xpcom are in my patch queue in bug 1306327, although some of the cleanup in nsXPCOMPrivate.h isn't, but I'm going to add that there. That leaves the changes to nscore.h and xrecore.h, along sdp_file_parser.cpp and xptcall.h.
Attachment #8818297 - Flags: review?(mh+mozilla)
> This patch requires the ALLOWED_XPCOM_GLUE stuff in emitter.py to be removed
> first, otherwise, the build fails during spline reticulation.

Indeed, I have a backout of all that.
Attachment #8826768 - Flags: review?(mh+mozilla)
Comment on attachment 8826768 [details]
Bug 1306329 - Backout 621aa115c3df (bug 1316450).

https://reviewboard.mozilla.org/r/104640/#review105384
Attachment #8826768 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8826769 [details]
Bug 1306329 - Things that depend on xul should no longer link the XPCOM glue library.

https://reviewboard.mozilla.org/r/104642/#review105386
Attachment #8826769 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8826770 [details]
Bug 1306329 - Don't build the dependent XPCOM glue.

https://reviewboard.mozilla.org/r/104644/#review105388
Attachment #8826770 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8826771 [details]
Bug 1306329 - Stop exporting XPCOM and XUL symbols.

https://reviewboard.mozilla.org/r/104646/#review105390
Attachment #8826771 - Flags: review?(mh+mozilla) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/ad4e531c7070
Backout 621aa115c3df (bug 1316450). r=glandium
https://hg.mozilla.org/integration/autoland/rev/377ca1419f1a
Things that depend on xul should no longer link the XPCOM glue library. r=glandium
https://hg.mozilla.org/integration/autoland/rev/6bb17b9a62d8
Don't build the dependent XPCOM glue. r=glandium
https://hg.mozilla.org/integration/autoland/rev/1c2f51ce3faf
Stop exporting XPCOM and XUL symbols. r=glandium
How does removing code add hazards?
Flags: needinfo?(sphink)
Blocks: 1331863
My guess here is that changing the XPCOM functions from C to C++ messed up these exceptions: http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/js/src/devtools/rootAnalysis/annotations.js#179

Unfortunately I don't see any docs on how to set up this build locally; https://developer.mozilla.org/en-US/docs/Mozilla/Continuous_integration links to a bug but without instructions. I might try naively changing these to the equivalent C++ name from c++filt and see if that works on try.
Flags: needinfo?(benjamin)
I pushed a version of the last patch that keeps the extern "C" (while still getting rid of the export). If that works, I'll push that and figure out the rest later. It isn't critical to getting rid of the exports.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5425a3627cc6a576908efdfb73fa6a616b877
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/8f4893e390c3
Backout 621aa115c3df (bug 1316450). r=glandium
https://hg.mozilla.org/integration/autoland/rev/15c053fffd35
Things that depend on xul should no longer link the XPCOM glue library. r=glandium
https://hg.mozilla.org/integration/autoland/rev/65fcf5e91cf0
Don't build the dependent XPCOM glue. r=glandium
https://hg.mozilla.org/integration/autoland/rev/a5aa102e8f0f
Stop exporting XPCOM and XUL symbols. r=glandium
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: needinfo?(sphink)
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d62adf74d89
Backout 621aa115c3df (bug 1316450). r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/28ca2b49e4ca
Things that depend on xul should no longer link the XPCOM glue library. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc53f232f8b
Don't build the dependent XPCOM glue. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/5abe47a8bfe0
Stop exporting XPCOM and XUL symbols. r=glandium
Blocks: 1332631
Blocks: 1332639
This fixing of this bug apparently causes Firefox 53 to crash whenever old extensions, which are not prevented from loading, invoke any of the removed symbols.  Here is a typical crash report:

https://crash-stats.mozilla.com/report/index/bp-9b566232-4dd8-448d-90a7-855cf2170407

The crash is very easy to understand.  It occurs because the XPCOM dylib in my old extension calls _NS_GetServiceManager.  Because there is no such symbol in Firefox 53, Firefox crashes.

STEPS TO REPRODUCE:

• Get on a Mac running macOS 10.12.
• Download and install BookMacster version 2.2.18 from here:

https://sheepsystems.com/bookmacster/BookMacster.2.2.18.zip

(Do not get it from the link given on my website because I hope to soon have it replaced with a newer version 2.3 which contains a new WebExtension instead.)

• Launch Firefox 53.0b9 (64-bit) Build ID 20170403072723.
• Launch BookMacster.  Close or cancel any windows that open.
• Click in the menu: BookMacster > Manage Browser Extensions.  A window will open.
• In the top "Syncing" section, row for "Firefox", click button "Install", and follow the instructions given.
• Relaunch Firefox 53.0b9.
• Click in the menu Tools > Add-Ons and verify that you have Sheep Systems Sync Extension version 330 loaded.
• Return to the Manage Browser Extensions window in BookMacster and click "Test" in the row for Firefox.  This sends a message to the .dylib in my extension.

RESULT:

Firefox 53 will crash immediately.

Now, of course the way it will happen in real life is that users who have been running version 2.2.18 of BookMacster or similar apps, and have installed my old extension, and been happily using it in Firefox 52 or earlier, and have not updated BookMacster before the week of April 18 because they have automatic updating turned off, will find that, after Firefox updates itself to version 53, Firefox will crash whenever they add, delete or change a bookmark in Firefox, or perform certain operations in my app.

Similar crashes can be expected to occur with any other extensions still out in the wild which have XPCOM dylibs.

Please note that there is apparently nothing in Firefox 53 which prevents my old extension from loading.  The install.rdf in my old extension specifies the maxVersion for Firefox = 51.0, but I read somewhere that Firefox has never enforced this :(
Jerry, can you file a new bug for this issue (make it block this one).

I would have expected the extension not to load at all. How come it still loads if there are missing symbols? Are they weakly linked?
Depends on: 1354395
OK, Mike.  I have now filed Bug 1354395, and added a comment there regarding the weak linking question.
Depends on: 1278282
You need to log in before you can comment on or make changes to this bug.