Closed
Bug 1160724
Opened 9 years ago
Closed 9 years ago
Undefined var(--space-above-tabbar) in browser/themes/osx/browser.css
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: ttaubert, Assigned: heycam)
References
Details
Attachments
(2 files, 2 obsolete files)
1021 bytes,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
3.32 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
This has been around for a very long time. My log is full of it when running sessionstore tests locally.
Reporter | ||
Comment 1•9 years ago
|
||
I think this might actually be a CSS parser bug.
Reporter | ||
Updated•9 years ago
|
Component: Theme → CSS Parsing and Computation
Product: Firefox → Core
Reporter | ||
Comment 2•9 years ago
|
||
This is a test you can simply run and observe how we end up reporting a CSS parser error here: https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#2609 The test itself runs fine, looks like to computed value is correct. That baffles me a little but then this bug was filed to get rid of the console messages. Cameron, David, any idea what's happening here?
Attachment #8600623 -
Flags: feedback?(dbaron)
Attachment #8600623 -
Flags: feedback?(cam)
Reporter | ||
Comment 3•9 years ago
|
||
While looking into this I *might* have found a bug, which unfortunately doesn't fix the issue itself but might cause future issues? It seems like nsStyleVariables should copy mVariables from aSource in the constructor that takes a source argument.
Attachment #8600624 -
Flags: review?(dbaron)
Attachment #8600624 -
Flags: review?(cam)
Reporter | ||
Comment 4•9 years ago
|
||
BTW, the test case might look arbitrary but we're actually hitting exactly that scenario with browser windows, at least on OS X. I reduced the test case from browser.xul and its dependencies.
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #2) > Cameron, David, any idea what's happening here? Maybe we are computing styles for the <vbox> before the style sheet has finished loading (I don't think we load data: URL style sheet links synchronously?) and then restyle once it has?
Assignee | ||
Comment 6•9 years ago
|
||
It's hard to know what to do with warnings about invalid variable references. I wonder if it would make sense to suppress them if there are outstanding style sheet loads?
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8600624 [details] [diff] [review] 0002-Bug-1160724-Copy-mVariables-in-nsStyleVariables-s-co.patch Review of attachment 8600624 [details] [diff] [review]: ----------------------------------------------------------------- Variables is a bit different from all other style structs, in that in nsRuleNode::ComputeVariablesData we explicitly copy in all of the inherited variables into our new nsStyleVariables struct. With other structs, we construct a copy of the inherited struct (the copy constructor call passing in parentdata_ in COMPUTE_START_INHERITED) and only overwrite properties that had specified values from the rules we're looking at. Because we copy in the inherited variables anyway, any copy construction we do will be wasted work. (We also invoke the copy constructor with the aStartStruct argument in COMPUTE_START_INHERITED, but because we never consider a Variables struct as being fully specified, aStartStruct will only ever be the root rule node's struct (and will therefore be empty).) But the current copy constructor is indeed confusing. I think we should at least add a comment to the method pointing out that it's deliberate we don't copy over mVariables. Or -- and maybe this is better -- make the copy constructor copy over mVariables, but also change COMPUTE_START_INHERITED so that it never invokes the copy constructor with parentdata_ if we are computing a Variables struct.
Attachment #8600624 -
Flags: review?(cam) → review-
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #5) > Maybe we are computing styles for the <vbox> before the style sheet has > finished loading (I don't think we load data: URL style sheet links > synchronously?) and then restyle once it has? Ah, this is actually an issue I've encountered before, but I can't find the other bug where I would have mentioned it. In Element::GetBindingURL, we resolve a new style context just for the purpose of getting the value of -moz-binding. We pass in nullptr for the parent style context, which means we don't have access to any inherited style information. Thus, when we resolve the value of transform with its variable reference, we get an invalid value. This doesn't matter, since we're only looking at -moz-binding anyway.
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8600623 [details] [diff] [review] 0001-Bug-1160724-Add-test-to-trigger-CSS-parser-error-mes.patch Can you make this test fail if the warning is emitted?
Attachment #8600623 -
Flags: feedback?(dbaron)
Attachment #8600623 -
Flags: feedback?(cam)
Attachment #8600623 -
Flags: feedback+
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5494e3e0a6bd
Attachment #8600624 -
Flags: review?(dbaron)
Comment on attachment 8600759 [details] [diff] [review] Resolve a properly parented style context when looking up -moz-binding of a display:none XUL/plugin element. r=dbaron, but keep an eye out for performance regressions
Attachment #8600759 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 13•9 years ago
|
||
That didn't quite work; trying again with a tweaked patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=715eaf4087c6
Reporter | ||
Comment 14•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #7) > Because we copy in the inherited variables anyway, any copy construction we > do will be wasted work. I see. Thanks for explaining!
Reporter | ||
Updated•9 years ago
|
Attachment #8600624 -
Attachment is obsolete: true
Reporter | ||
Comment 15•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #9) > Can you make this test fail if the warning is emitted? I wish, but I don't know how. Can't see the warning in the console, guess a console log listener wouldn't work then.
Reporter | ||
Comment 16•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #13) > That didn't quite work; trying again with a tweaked patch: This version works great locally. Thank you for the quick solution!
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #15) > I wish, but I don't know how. Can't see the warning in the console, guess a > console log listener wouldn't work then. That might be an existing problem where some variable resolution re-parses don't have anywhere to send their warnings. Never mind, we'll do without the test.
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #7) > But the current copy constructor is indeed confusing. I think we should at > least add a comment to the method pointing out that it's deliberate we don't > copy over mVariables. Or -- and maybe this is better -- make the copy > constructor copy over mVariables, but also change COMPUTE_START_INHERITED so > that it never invokes the copy constructor with parentdata_ if we are > computing a Variables struct. Tim, did you want to look at this? If so, a separate bug would be better for it.
Flags: needinfo?(ttaubert)
Reporter | ||
Comment 19•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #18) > Tim, did you want to look at this? If so, a separate bug would be better > for it. Don't have the time to dive into the CSS parser, sorry :) Just stumbled upon it at my weekend investigation.
Flags: needinfo?(ttaubert)
Reporter | ||
Comment 20•9 years ago
|
||
Using an nsIConsoleListener does actually work. Fails without your patch and succeeds with your patch applied.
Attachment #8600623 -
Attachment is obsolete: true
Attachment #8600815 -
Flags: review?(cam)
Comment 21•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #8) > (In reply to Cameron McCormack (:heycam) from comment #5) > > Maybe we are computing styles for the <vbox> before the style sheet has > > finished loading (I don't think we load data: URL style sheet links > > synchronously?) and then restyle once it has? > > Ah, this is actually an issue I've encountered before, but I can't find the > other bug where I would have mentioned it. In Element::GetBindingURL, we > resolve a new style context just for the purpose of getting the value of > -moz-binding. We pass in nullptr for the parent style context, which means > we don't have access to any inherited style information. Thus, when we > resolve the value of transform with its variable reference, we get an > invalid value. This doesn't matter, since we're only looking at > -moz-binding anyway. Could this be Bug 1093260?
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #21) > Could this be Bug 1093260? Thanks yes that's it.
Assignee | ||
Comment 23•9 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #19) > Don't have the time to dive into the CSS parser, sorry :) Just stumbled upon > it at my weekend investigation. No problem. :) Filed bug 1161325 for that.
Assignee | ||
Updated•9 years ago
|
Attachment #8600815 -
Flags: review?(cam) → review+
Comment 24•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/06ac99c8221f https://hg.mozilla.org/integration/mozilla-inbound/rev/8123996547d6
Comment 25•9 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #0) > This has been around for a very long time. My log is full of it when running > sessionstore tests locally. Yeah, I was away or I would have duped this... Now that I'm back, let me find the other bug about this.
Comment 26•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #25) > (In reply to Tim Taubert [:ttaubert] from comment #0) > > This has been around for a very long time. My log is full of it when running > > sessionstore tests locally. > > Yeah, I was away or I would have duped this... Now that I'm back, let me > find the other bug about this. ... and I should read all my bugmail before commenting. I was thinking of 1093260. Move along, nothing to see here!
Comment 27•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/06ac99c8221f https://hg.mozilla.org/mozilla-central/rev/8123996547d6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•