Closed Bug 1160724 Opened 5 years ago Closed 5 years ago

Undefined var(--space-above-tabbar) in browser/themes/osx/browser.css

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ttaubert, Assigned: heycam)

References

Details

Attachments

(2 files, 2 obsolete files)

This has been around for a very long time. My log is full of it when running sessionstore tests locally.
I think this might actually be a CSS parser bug.
Component: Theme → CSS Parsing and Computation
Product: Firefox → Core
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)
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)
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.
(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?
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?
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-
(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.
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: nobody → cam
Status: NEW → ASSIGNED
Attachment #8600759 - 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+
That didn't quite work; trying again with a tweaked patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=715eaf4087c6
(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!
Attachment #8600624 - Attachment is obsolete: true
(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.
(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!
(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.
(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)
(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)
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)
(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?
See Also: → 1093260
(In reply to Brian Grinstead [:bgrins] from comment #21)
> Could this be Bug 1093260?

Thanks yes that's it.
(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.
Attachment #8600815 - Flags: review?(cam) → review+
(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.
(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!
https://hg.mozilla.org/mozilla-central/rev/06ac99c8221f
https://hg.mozilla.org/mozilla-central/rev/8123996547d6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1163257
Depends on: 1383142
You need to log in before you can comment on or make changes to this bug.