Closed
Bug 469244
Opened 16 years ago
Closed 16 years ago
[ARM, and maybe other rare architectures] various css properties values are not able to be set
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b4
People
(Reporter: jmaher, Assigned: dbaron)
References
Details
(Keywords: fixed1.9.1)
Attachments
(4 files)
1.91 KB,
text/html
|
Details | |
958 bytes,
text/plain
|
Details | |
12.69 KB,
patch
|
Details | Diff | Splinter Review | |
2.88 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
dbaron
:
approval1.9.1+
|
Details | Diff | Splinter Review |
I have a list of 28 css properties which are not able to return a value after setting it via setProperty. Many other properties work in this method.
This was found while running mochitests:
layout/styletest/test/initial_storage.html
layout/styletest/test/inherit_storage.html
If we do not support these inside of fennec, then we need to modify the testcase to ignore these values when running against fennec.
Here is the list of properties:
"azimuth"
"cue"
"cueAfter"
"cueBefore"
"elevation"
"font"
"fontStretch"
"marks"
"orphans"
"page"
"pageBreakInside"
"pause"
"pauseAfter"
"pauseBefore"
"pitch"
"pitchRange"
"richness"
"size"
"speak"
"speakHeader"
"speakNumeral"
"speakPunctuation"
"speechRate"
"stress"
"voiceFamily"
"volume"
"widows"
Another thing to note is this only fails on the maemo device, but appears to run just fine under fennec on the linux desktop.
Reporter | ||
Comment 1•16 years ago
|
||
this also causes 344 tests to fail on layout/style/test/test_value_storage.html
Comment 2•16 years ago
|
||
this is very strange. we don't explicitly turn off support for any css.
Reporter | ||
Comment 3•16 years ago
|
||
could it be we can set them, but not retrieve them?
Reporter | ||
Comment 4•16 years ago
|
||
moving bug to relevant component.
Component: General → Style System (CSS)
Product: Fennec → Core
QA Contact: general → style-system
Assignee | ||
Comment 5•16 years ago
|
||
I vaguely remember proposals to #ifdef some of this code, but I don't actually *see* any such ifdefs.
Where does the fennec build configuration live?
Reporter | ||
Comment 6•16 years ago
|
||
I am not sure what the build configuration is. Is this a mozconfig file you are looking for?
Assignee | ||
Comment 7•16 years ago
|
||
That would be a good start, yes.
Reporter | ||
Comment 8•16 years ago
|
||
I can also provide binaries if that would be helpful
Assignee | ||
Updated•16 years ago
|
Attachment #370219 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 9•16 years ago
|
||
Were there any custom patches or branches used in creating the build that shows this bug?
Reporter | ||
Comment 10•16 years ago
|
||
We use these instructions:
https://wiki.mozilla.org/Mobile/Build/Fennec
This uses:
hg.mozilla.org/mozilla-central
hg.mozilla.org/mobile-browser
Since this only fails on the device and not a build of the desktop version, it could be related to a resource issue or something that depends on an os specific library.
Those are just my observations and I am open to trying things out to help debug the problem further.
Assignee | ||
Comment 11•16 years ago
|
||
I don't think there's anything else I can do without debugging it on the device where the problem is happening. I don't consider this a high enough priority to figure out how to set up a build environment, and I'm not even sure what debugging techniques would apply if I had one.
This also seems like something that would result from a patch to intentionally disable these, so I'm a little skeptical that the build showing this doesn't have extra patches that I'm not seeing.
Assignee | ||
Comment 12•16 years ago
|
||
For what it's worth, this list of properties is exactly the list of properties that use CSS_PROP_BACKENDONLY (plus shorthands that contain them). However, I don't see how somebody could just redefine that macro without causing compilation errors in the parser, so it really seems like there's an additional patch somewhere.
Reporter | ||
Comment 13•16 years ago
|
||
to double check everything, this morning I did a fresh checkout of mozilla-central and mobile-browser and used the mozconfig file attached to this bug. There are no patches or any other configuration. I also blew away any debug/release directories sitting around to clean up my machine and build environment.
I did a "make -f client.mk build" inside scratchbox and when finished, I did a make package, make package-tests and scp'd to my device.
I ran the mochitests for layout/style/tests/ and the results are:
Passed: 27777
Failed: 464
Todo: 58
The 464 failures are the same ones we are talking about in this bug. While the test was running, I did verify that I was using fennec (used ps from the ssh session as well as visual verification on the screen).
I searched in the code for some way that cross compiling to maemo could ignore the CSS_PROP_BACKENDONLY properties. I didn't have any luck. I do know that all of the properties listed above appear to be related to sound and maybe there is some issue on maemo/hildon/scratchbox with libraries, plugins, native themes related to sound?
If there is something I can do to instrument the code or test differently, I would be more than happy to do that.
Assignee | ||
Comment 14•16 years ago
|
||
(In reply to comment #13)
> I searched in the code for some way that cross compiling to maemo could ignore
> the CSS_PROP_BACKENDONLY properties. I didn't have any luck. I do know that
> all of the properties listed above appear to be related to sound and maybe
> there is some issue on maemo/hildon/scratchbox with libraries, plugins, native
> themes related to sound?
No, since the properties aren't actually hooked up to anything. (Also, page, page-break-inside, orphans, and widows are not related to sound.)
Assignee | ||
Comment 15•16 years ago
|
||
Could you tell me what gets printed out when you run the reduced testcase above, with this patch compiled in?
(This patch prints debugging output only about the 'pitch' property; I just picked one of the properties to reduce the amount of output.)
Reporter | ||
Comment 16•16 years ago
|
||
so I have this working on my device, and here is the results that I get:
WARNING: Unable to test style tree integrity -- no content node: file /home/mozilla/src/layout/base/nsCSSFrameConstructor.cpp, line 7612
++DOMWINDOW == 11 (0x40424630) [serial = 14] [outer = 0x40424280]
/home/mozilla/src/layout/style/nsCSSParser.cpp:1070: Parsing declaration of 'pitch' property (id)
/home/mozilla/src/layout/style/nsCSSParser.cpp:5719: Parsing 'pitch' property
/home/mozilla/src/layout/style/nsCSSParser.cpp:5277: Appending value for 'pitch' property
/home/mozilla/src/layout/style/nsCSSParser.cpp:1092: Parsing declaration of 'pitch' property succeeded
/home/mozilla/src/layout/style/nsCSSDataBlock.cpp:810: Compressing value for 'pitch' property.
/home/mozilla/src/layout/style/nsDOMCSSDeclaration.cpp:177: GetPropertyValue for 'pitch' property (string)
/home/mozilla/src/layout/style/nsDOMCSSDeclaration.cpp:81: GetPropertyValue for 'pitch' property (id)
/home/mozilla/src/layout/style/nsCSSDeclaration.cpp:483: nsCSSDeclaration::GetValue for 'pitch' property
/home/mozilla/src/layout/style/nsCSSDeclaration.cpp:135: nsCSSDeclaration::AppendValueToString for 'pitch' property
/home/mozilla/src/layout/style/nsCSSDataBlock.cpp:371: Checking for storage for 'pitch' property.
++WEBSHELL 0x4045f6c0 == 6
++DOMWINDOW == 12 (0x404249b0) [serial = 15] [outer = (nil)]
++DOMWINDOW == 13 (0x40424b70) [serial = 16] [outer = 0x40424980]
--WEBSHELL 0x4045f6c0 == 5
/home/mozilla/src/layout/style/nsDOMCSSDeclaration.cpp:81: GetPropertyValue for 'pitch' property (id)
/home/mozilla/src/layout/style/nsCSSDeclaration.cpp:483: nsCSSDeclaration::GetValue for 'pitch' property
/home/mozilla/src/layout/style/nsCSSDeclaration.cpp:135: nsCSSDeclaration::AppendValueToString for 'pitch' property
/home/mozilla/src/layout/style/nsCSSDataBlock.cpp:371: Checking for storage for 'pitch' property.
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /home/mozilla/src/toolkit/components/places/src/nsFaviconService.cpp, line 697
--DOMWINDOW == 12 (0x40424b70) [serial = 16] [outer = 0x40424980] [url = about:blank]
--DOMWINDOW == 11 (0x404249b0) [serial = 15] [outer = (nil)] [url = chrome://global/content/commonDialog.xul]
The area in the middle (where the DOMWINDOW lines are) is where I put an alert() to tell me the value is invalid, then after it I remove the stored value.
Assignee | ||
Comment 17•16 years ago
|
||
Thanks. That helps. I think I see the bug now, although I'm surprised that we didn't hit problems with the expression 1 << nsStyleStructID(-1) on any other architectures that we run on.
(I'm presuming that what you're seeing this on is ARM Linux.)
Assignee: nobody → dbaron
Priority: -- → P2
Hardware: Other → ARM
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 18•16 years ago
|
||
Could you try this and see if it fixes the problem?
Assignee | ||
Comment 19•16 years ago
|
||
Comment on attachment 370452 [details] [diff] [review]
fix
Requesting review whether or not this fixes the problem (although I strongly suspect it will), since we need to fix this anyway to avoid relying on behavior undefined according to C99 (ISO/IEC 9899:1999), section 6.5.7 clause 3: "If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined."
Attachment #370452 -
Flags: superreview?(bzbarsky)
Attachment #370452 -
Flags: review?(bzbarsky)
Updated•16 years ago
|
Attachment #370452 -
Flags: superreview?(bzbarsky)
Attachment #370452 -
Flags: superreview+
Attachment #370452 -
Flags: review?(bzbarsky)
Attachment #370452 -
Flags: review+
Reporter | ||
Comment 20•16 years ago
|
||
after running a test with this on the Maemo device we pass all the tests listed above! Thanks for the patch and onto investigating other failures.
Assignee | ||
Comment 21•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Summary: various css properties values are not able to be set → [ARM, and maybe other rare architectures] various css properties values are not able to be set
Assignee | ||
Updated•16 years ago
|
Attachment #370452 -
Attachment description: possible fix → fix
Attachment #370452 -
Flags: approval1.9.1?
Assignee | ||
Comment 22•16 years ago
|
||
And sorry about not believing that this was actually a bug in the code rather than something being commented out.
Reporter | ||
Comment 23•16 years ago
|
||
no worries...99.9% chance that it was a patch or something else with the general build, better to do your due diligence up front.
Assignee | ||
Updated•16 years ago
|
Attachment #370452 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 24•16 years ago
|
||
Comment on attachment 370452 [details] [diff] [review]
fix
a1.9.1=roc (we did the approval triage via email, and I'm marking)
Assignee | ||
Comment 25•16 years ago
|
||
Keywords: fixed1.9.1
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b4
You need to log in
before you can comment on or make changes to this bug.
Description
•