Crashes when I start Firefox on Sparc64 [@ nsTemplateRule.operator=]

RESOLVED WORKSFORME

Status

()

--
major
RESOLVED WORKSFORME
13 years ago
8 years ago

People

(Reporter: ganesh.mozilla, Unassigned)

Tracking

({64bit, crash})

1.8 Branch
Other
All
64bit, crash
Points:
---
Bug Flags:
blocking1.8.1 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs retesting on Sparc64], crash signature)

Attachments

(6 attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: 

When I try to launch 64-bit "firefox-1.5", the browser comes up and stays for a while. But after some time it crashed with a segmentation fault every time (Even if you don't do anything in the browser). Stack trace shows that "FatalSignalHandler" is called from "nsProfileLock.cpp" before it crashes.

Reproducible: Always

Actual Results:  
firefox-1.5 crashes with a segmentation fault.

Expected Results:  
Browser should not be crashed.
(Reporter)

Comment 1

13 years ago
Created attachment 210134 [details]
Stack trace

Attaching the stack trace.
(Reporter)

Updated

13 years ago
OS: Other → AIX

Comment 2

13 years ago
for reference, the fatalsignalhandler is called *after* it crashes precisely because it has *crashed*
Component: General → XP Toolkit/Widgets: XUL
Keywords: crash
Product: Firefox → Core
QA Contact: general → xptoolkit.xul
Summary: crashes with a segmentation fault when I start 64-bit "firefox-1.5" → crashes with a segmentation fault when I start 64-bit "firefox-1.5" [@ nsTemplateRule.operator=]
Version: unspecified → 1.8 Branch

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: 64bit
(Reporter)

Comment 3

13 years ago
At the time of segmentation fault, the value of aSet is NULL (code snippet given below). So accessing its member causes the segmentation fault. The aSet (NULL) value is passed to this function from RecomputeBindings function in "nsTemplateRule.cpp".

http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsRuleNetwork.h#451

 nsAssignmentSet& operator=(const nsAssignmentSet& aSet) {
         NS_IF_RELEASE(mAssignments);
         mAssignments = aSet.mAssignments;
         NS_IF_ADDREF(mAssignments);
         return *this; }


From RecomputeBindings function the NULL (aMatch->mInstantiation.mAssignments) is passed to the above function since, this function itself gets a NULL value from SynchronizeAll  function in nsTemplateMatchSet.h.

http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsTemplateRule.cpp#224

     // Truncate the match's assignments to only include
     // assignments made via condition tests. We'll add back
     // assignments as they are recomputed.

      aMatch->mAssignments = aMatch->mInstantiation.mAssignments;


In SynchronizeAll  function, NULL is gettting from nsTemplateMatch* get() in nsTemplateMatchSet.h. This get () function returns the value depending on mInlineMatches.mCount and kMaxInlineMatches values.

http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsTemplateMatchSet.h#269

nsTemplateMatch* get() const {
            return mSet->mStorageElements.mInlineMatches.mCount > PRUint32(kMaxInlineMatches)
                 ? mTableEntry->mMatch
                 : *mInlineEntry; }


For InlineMatches structure, I can see some comments written like there may be some problem on 64 bit system. 

http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsTemplateMatchSet.h#221

Since we are building "64-bit browser", I suspect this comment is related with this problem. 

Comment 4

13 years ago
With the comments written at http://lxr.mozilla.org/seamonkey/source/content/xul/templates/src/nsTemplateMatchSet.h#221,  It seems to be the 'mCount' of 'InlineMatches' structure overlaps 'ops' member of PLDHashTable. So, I believe both of them should be in same size. In 32 bit compilation, both are 4 bytes and in 64 bit compilation 'mCount' is 4 bytes and 'ops' is 8 bytes. With the below change, I don't see the seg fault. Can somebody familiar with this code comment on this?

struct InlineMatches {
         PRUint32         mCount;  =======>PRUint64 mCount;
         nsTemplateMatch* mEntries[kMaxInlineMatches];
     };

Comment 5

13 years ago
you can't change it like that as PRUint64 and friends require different
operators. given that the code can't possibly use a 64bit number, i'd rather we
didn't waste the space on 32bit platforms or the code instructions to properly
pretend to deal w/ a 64bit number.

I think the right way to do it is probably another union

 struct InlineMatches {
          union {
              PRUint32         mCount;
              void*            unusedForAlignment;
          };
          nsTemplateMatch* mEntries[kMaxInlineMatches];
      };

making the union anonymous will probably earn me a compilation warning, so it
should probably be named and properly accessed everywhere, but i don't feel
like doing that.

Comment 6

13 years ago
Did the assertion at line 225 of nsTemplateMatchSet.cpp fire?
(Reporter)

Comment 7

13 years ago
Yes, it hits the Assertion at line 225 of nsTemplateMatchSet.cpp. And also because of this, it fires the Assertion at line 1226 of nsXULContentBuilder.cpp.
Note that on trunk this file is getting removed altogether in bug 285631.  So if we do anything here it's branch-only.

Neil Deakin, any thoughts on how to best fix this on branch?  I think we certainly want the fix for 1.8.1....
Depends on: 285631

Comment 9

13 years ago
Hi, Any plans of fixing this problem?
Flags: blocking1.8.1?

Comment 10

13 years ago
rupesh: does my suggestion work? if so, could you attach a patch that implements it?

Comment 11

13 years ago
Timeless, I have tried your suggession with anonymous union, but still seeing the same seg fault.
(In reply to comment #11)
> Timeless, I have tried your suggession with anonymous union, but still seeing
> the same seg fault.
> 

Rupesh - can you verify the memory layout with timeless' suggestion?

His idea should work - forcing things to be 64 bit on 64 bit platforms, but 32bit on 32bit platforms.

Are you sure it is the same crash?

Comment 13

13 years ago
Not going to block 1.8.1 for this, but we'll happily consider a baked-on-trunk patch.
Flags: blocking1.8.1? → blocking1.8.1-

Comment 14

12 years ago
It fires assertion here at 225 of nsTemplateMatchSet.cpp on sparc64, but does not crash (firefox 2.0.0.1). Live bookmarks and some other things just do not work because of that.

Comment 15

12 years ago
Created attachment 255581 [details]
Output compiled with --enable-debug

Comment 16

12 years ago
Created attachment 255584 [details]
Configure log

Now rebuilding the it with the change timeless suggested. Will let you know as soon as it finishes.

Updated

12 years ago
Severity: critical → major
OS: AIX → All

Comment 17

12 years ago
Created attachment 255760 [details] [diff] [review]
patch-content_xul_templates_src_nsTemplateMatchSet_h

Tested timeless' suggestion -- does not fix it.

The patch i'm attaching fixes for us some major problems on sparc64 (e.g. live bookmarks). This was a long-time problem (appeared since 1.5, i suppose).

It is not reproducible for all 64bit architectures (tried on alpha and amd64); so i'm assuming the problem might be mixing 32/64-bit access (usually works on little-endian; but fails on big-endian). So i didn't touch other archs (who touch things which just work).

This should be common for all architectures; however i've tried everything under OpenBSD only. So if you are against committing it, please just surround it with extra __OpenBSD__ ifdefs, because we are using this in our ports-tree; would be nice to shrink our patches with every release.

Comment 18

12 years ago
Comment on attachment 255760 [details] [diff] [review]
patch-content_xul_templates_src_nsTemplateMatchSet_h

stop this.

please post enough make foo.i output to show *why* my suggestion doesn't work. you're free to zip the foo.i's together and attach a single zip.
Attachment #255760 - Flags: review-

Comment 19

12 years ago
Sure. Will do.

Comment 20

12 years ago
Created attachment 255903 [details]
nsTemplateMatchSet_1.i -- the one with hardcoded PRUint64

Comment 21

12 years ago
Created attachment 255905 [details]
nsTemplateMatchSet_2.i -- the one timeless suggested

Comment 22

12 years ago
timeless, sorry if that's too less... Just let me know if/which ones do you need.
The sparc64.. I don't have it. I'm asking a friend of mine so it's limited for me too.

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets

Comment 24

9 years ago
Martynas, do newer versions of Firefox work for you (on Sparc64)?  I see bug reports for Sparc64 other than startup crashes, so it's likely that it works now.
Summary: crashes with a segmentation fault when I start 64-bit "firefox-1.5" [@ nsTemplateRule.operator=] → Crashes when I start Firefox on Sparc64 [@ nsTemplateRule.operator=]

Updated

9 years ago
Whiteboard: [needs retesting on Sparc64]

Comment 25

9 years ago
New versions (firefox3+) work fine on sparc64 and don't need the nsTemplateMatchSet.h quirk.

Comment 26

9 years ago
Yay!
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WORKSFORME
(Assignee)

Updated

8 years ago
Crash Signature: [@ nsTemplateRule.operator=]
You need to log in before you can comment on or make changes to this bug.