Closed Bug 594650 Opened 9 years ago Closed 9 years ago

Composer.dll is broken with disable-libxul builds

Categories

(Core :: ImageLib, defect)

x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b7

People

(Reporter: Felipe, Assigned: dholbert)

References

Details

Attachments

(3 files, 1 obsolete file)

Probably need to move the static initializer to a .cpp file.

Also, static initializers are bad!  Better hope Taras doesn't find this :-P
No longer blocks: 276431
Blocks: 276431
(In reply to comment #1)
> Probably need to move the static initializer to a .cpp file.
> 
> Also, static initializers are bad!  Better hope Taras doesn't find this :-P

Can we make this a singleton method or an extern?
fwiw this broke SeaMonkey (tinderbox) Debug Builds, no idea why our opt builds are still working of course.
So, here's a trivial patch to make this |extern|, but this triggers this build warning (in a firefox debug disable-libxul build):
../base/libgkbase_s.a(nsImageLoader.o): In function `nsImageLoader::FrameChanged(imgIContainer*, nsIntRect const*)':
/scratch/work/builds/mozilla-central/mozilla-central.10-05-11.21-07/obj/layout/base/../../../mozilla/layout/base/nsImageLoader.cpp:209: undefined reference to `mozilla::imagelib::kFullImageSpaceRect'
/usr/bin/ld: ../base/libgkbase_s.a(nsImageLoader.o): relocation R_386_GOTOFF against undefined hidden symbol `mozilla::imagelib::kFullImageSpaceRect' can not be used when making a shared object
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status
make[5]: *** [libgklayout.so] Error 1

What am I doing wrong?
I think that's symbol visibility biting you.

Try 
NS_COM const nsIntRect kFullImageSpaceRect(0, 0, INT_MAX, INT_MAX);
You mean, just adding NS_COM to the second chunk in attachment 473403 [details] [diff] [review]?
Sadly, that doesn't fix it -- I still get the "relocation R_386_GOTOFF against undefined hidden symbol `mozilla::imagelib::kFullImageSpaceRect' can not be used when making a shared object". :(

However, I do have a working fix that just replaces kFullImageSpaceRect with a static method
 static nsIntRect BuildMaxSizedIntRect();
inside of the "nsIntRect" class definition (in nsRect.h)...
This seems to fix it -- taras, what do you think?
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #473436 - Flags: review?(tglek)
I'm heading to bed soon, but assuming this gets review, feel free to push it on my behalf, if the seamonkey bustage needs quick fixing.  (a=bustage-fix-for-blocker is fine for approval, I think)
(In reply to comment #3)
> fwiw this broke SeaMonkey (tinderbox) Debug Builds, no idea why our opt builds
> are still working of course.
I think the compiler gets to inline the constructor in the opt case.
Comment on attachment 473436 [details] [diff] [review]
second try: add nsIntRect::GetMaxSizedIntRect

>-    observer->FrameChanged(this, &kFullImageSpaceRect);
>+    const nsIntRect fullSpace = nsIntRect::GetMaxSizedIntRect();
>+    observer->FrameChanged(this, &fullSpace);
Why are you making a copy of the rect?
Oh, good catch -- that's cruft from an earlier version of the patch.  (The old patch returned a freshly constructed nsIntRect instead of a reference to a static nsIntRect.)

fixing...
This fixes the unnecessary copy that Neil brought up.
Attachment #473436 - Attachment is obsolete: true
Attachment #473447 - Flags: review?(tglek)
Attachment #473436 - Flags: review?(tglek)
Attachment #473447 - Flags: review?(roc)
Comment on attachment 473447 [details] [diff] [review]
second try v2: add nsIntRect::GetMaxSizedIntRect

Thanks for fixing this.
Attachment #473447 - Flags: review?(tglek) → review+
http://hg.mozilla.org/mozilla-central/rev/cfb46cb1043b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Please verify that the number of static initializers goes back to what it was before (1164 IIRC)
verified this fixes the --disable-libxul case.
Status: RESOLVED → VERIFIED
Codesighs graph for the landing that regressed this:
http://graphs.mozilla.org/graph.html#tests=[[31,1,97]]&sel=1283975291,1283981914
Val before regression: 31269700
Val at this changeset: 31296700
Codesighs increase:       27000

Codesighs Graph for this landing:
http://graphs.mozilla.org/graph.html#tests=[[31,1,97]]&sel=1284046476,1284051444
Val before this changeset: 29751200
Val at this changeset:     29734300
Codesighs reduction:          16900

So, this push fixed 62% of the Codesighs increase associated with bug 276431.
(In reply to comment #17)
> Codesighs graph for the landing that regressed this:
> http://graphs.mozilla.org/graph.html#tests=[[31,1,97]]&sel=1283975291,1283981914
> Val before regression: 31269700
> Val at this changeset: 31296700
> Codesighs increase:       27000
er, sorry -- in that first chunk, "Val at this changeset" should read "Val right after bug 276431 landed", or something like that.
It also dropped num_ctors back down to what it was + 4.  Looks like we're good.
Hooray! I was going to ask how to check that, to answer comment 15.  Thanks for checking it for me. :)
You need to log in before you can comment on or make changes to this bug.