Closed Bug 1353177 Opened 8 years ago Closed 7 years ago

stylo: preshints (mapped attributes) not handled correctly for elements not in a document

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bzbarsky, Unassigned)

References

Details

Attachments

(1 file)

poc patch
7.88 KB, patch
Details | Diff | Splinter Review 
Testcase: 

  <!DOCTYPE html>
  <script>
  var f = document.createElement("font");
  f.setAttribute("color", "green");
  alert(getComputedStyle(f).color);
  </script>

Should alert "rgb(0, 128, 0)" but alerts "rgb(0, 0, 0)".

The problem, of course, is that mapped attributes only go in the nsHTMLStyleSheet hashtable when the node is bound to a document (so GetComposedDoc becomes non-null).  But nsMappedAttributeElement::WalkContentStyleRules walks the mapped attributes unconditionally.  So resolving style for a node that's not in the document works fine in Gecko.  But in Servo, ServoStyleSet::ResolveMappedAttrDeclarationBlocks only computes servo declarations for things in the nsHTMLStyleSheet.  And then Gecko_GetHTMLPresentationAttrDeclarationBlock will return null, because even though we have an nsMappedAttributes it has no servo declaration block.

One thing we _could_ try is using the owner, not composed, doc for the nsHTMLStyleSheet.  We'd then be able to remove the BindToTree code that currently resets the nsHTMLStyleSheet, and will instead need something in adoption to do so, when the owner doc changes.  This might cause performance regressions in situations where we currently set multiple mapped attrs on an element that is not in the document.  I _think_ that doesn't happen in the most common mapped attr case, when coming from the non-fragment parser, but would need to check.  And we'd need to measure the innerHTML setter impact...

The other option, of course, is to have elements registering "dirty" nsMappedAttributes with the nsHTMLStyleSheet in some way....
Priority: -- → P2
I expect the fix for bug 1352898 fixed this.  Need to check.
Depends on: 1352898
It helped, but did not fix.

The new behavior is pretty bizarre.  The testcase in comment 0 still shows the bug, but if I just add a <body> element the problem goes away (!).  That's because if there is no <body> we never call ServoStyleSet::PreTraverseSync, but if there is one then we do.  I think because the style flush from the getComputedStyle call triggers frame construction for the <body> and hence a servo traversal, which _happens_ to update stuff.

If I modify the testcase to flush layout at the start of the <script>, before setting the "color" attr on the <font>, then even with a <body> things don't work.

If I add a PreTraverseSync call to ServoStyleSet::ResolveTransientStyle, the problem goes away.  That would match the fact that nsComputedDOMStyle::DoGetStyleContextNoFlush just calls ResolveTransientStyle.=

But walking all the mapped attrs every time ResolveTransientStyle is called is horrible.  We should either walk only the ones that are actually dirty or do that walk off layout flushes and make sure that addition of a mapped attrs without a servo decl block marks us as needing a layout flush or something.
Summary: stylo: preshints not handled correctly for elements not in a document → stylo: preshints (mapped attributes) not handled correctly for elements not in a document
Priority: P2 → --
Priority: -- → P3
bz, should this block shipping?
Flags: needinfo?(bzbarsky)
What worries me is that we now have really unpredictable behavior from comment 2.  Which means that it might randomly break pages sometimes but not in easily reproducible ways....  In particular, it could break pages in ways that don't necessarily get hit in testing.  :(

If we can figure out a reasonably safe fix I would much prefer we fixed this before shipping.  But if we don't manage that, I could be OK with shipping without this fixed as long a we make sure to fix in 58.
Flags: needinfo?(bzbarsky)
I cannot reproduce this issue with either the testcase from comment 0, or any variation suggested in comment 2.

I tried to check with an early version, but stylo is only built by default on Win64 since July (specifically bug 1366048), and version from that time already seem to work fine with the testcases, so I cannot tell which bug fixes this from mozregression.
This got "accidentally" fixed in bug 1352898.

Before that patch, this piece from comment 1 was true:

  "mapped attributes only go in the nsHTMLStyleSheet hashtable when the node is bound to a document"

but that's precisely what bug 1352898 changed.  In fact, this bug was filed during discussion in that bug, when I was looking through these codepaths.  That bug implemented the "using the owner, not composed, doc for the nsHTMLStyleSheet" fix.

That said, there's still a testcase here that fails with stylo while passing in Gecko:

  <!DOCTYPE html>
  <body>
    <script>
      document.body.offsetWidth;
      var doc = document.implementation.createHTMLDocument();
      var f = doc.createElement("font");
      f.setAttribute("color", "green");
      alert(getComputedStyle(f).color);
    </script>
  </body>

I don't know that we care, and we may want to file a separate bug on this anyway.
Attached patch poc patch — — Splinter Review 
After digging into the code, I'm guessing that passing the owner document of the element down to ServoStyleSet::ResolveMappedAttrDeclarationBlocks() and use the attribute stylesheet of that document would work, and this is a poc patch.

The change to DOMImplementation::CreateHTMLDocument is needed here because mAttrStyleSheet is only created in nsDocument::ResetStylesheetsToURI, but we never call into this function when document is created via DOMImplementation. I suspect that we should probably create mAttrStyleSheet when document is created.

bz, do you think this is the right direction towards fixing this? Is there any concern for this approach?

I'm wondering whether passing ServoStyleSet::mPresContext to CalculateMappedServoDeclarations when the sheet is owned by a different document could be a issue... Haven't looked into this closely.
Flags: needinfo?(bzbarsky)
Hmm.  Doing the "pass the document" thing makes some sense.  I'm not entirely sure about the way it picks a prescontext to use and whether that approach makes sense.

How does Gecko pass my testcase from comment 6 without creating an attr stylesheet?  I guess it just has a non-sheet-associated mapped attributes that it then walks without involving an attr stylesheet in any way?

I expect there may be other ways to create documents without ResetStylesheetsToURI getting called.  Most simply, |new Document|, right?

> I suspect that we should probably create mAttrStyleSheet when document is created.

It's not clear which is best here.  This would involve allocating a bunch of stuff... but maybe we allocate it anyway in common "data document" cases like XHR result documents.

I really do feel that all this, and the comment 6 testcase, should perhaps move to a different bug and we should resolve this one fixed.
Flags: needinfo?(bzbarsky)
Ok, so marking this FIXED, and filed bug 1400771.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Probably worth adding a test case, I guess?
Yes, that would be awesome.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: