Closed Bug 768756 Opened 12 years ago Closed 1 year ago

"ASSERTION: IsCSSEquivalentToHTMLInlineStyleSet failed" with parent document's selection in a removed iframe

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Assigned: ayg)

References

Details

(Keywords: assertion, testcase, Whiteboard: [fuzzblocker:editor-assertions])

Attachments

(6 files)

testcase
632 bytes, text/html
Details
warnings, stack trace
8.21 KB, text/plain
Details
Patch part 1 -- Use enum in nsHTMLCSSUtils for computed/specified style
26.23 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 2 -- Make nsComputedDOMStyle Init infallible and merge into constructor
12.87 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Patch part 3 -- Get rid of nsHTMLCSSUtils::GetDefaultViewCSS
14.66 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 4 -- Clean up nsHTMLCSSUtils::GetCSSInlinePropertyBase
5.21 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Attached file testcase — 
###!!! ASSERTION: IsCSSEquivalentToHTMLInlineStyleSet failed: 'NS_SUCCEEDED(res)', file editor/libeditor/html/nsHTMLCSSUtils.cpp, line 1112
Attached file warnings, stack trace — 

    
    

    
  
So basically the problem is that we let commands run in detached stuff, but detached stuff has no computed styles, so all sorts of things will break.  I don't think it's sane to try to get that to work.  Should we redefine "editable" so it only applies to things that descend from a document with non-null defaultView or something?  At least for the purposes of whether commands are enabled, I think things should probably be disabled if the selection isn't in a document that's attached to a window.

    
    

    
  
Yes, that makes sense.  Trying to edit things that are not attached to a document is crazy!

    
    

    
  
Also, can we spec that as well?

    
    

    
  
rniwa: please see comment 2 through 4.

    
    

    
  
So are you suggesting that editing commands only run if the document/frame has the view?

    
    

    
  
Yes.

    
    

    
  
Wait -- but we do compute style for detached iframes.  I think.  WebKit seems not to.  I guess there are some things that can't sensibly be computed, like height/width.  Test-case:

data:text/html,<!doctype html>
<iframe></iframe>
<script>
function getFontWeight(compStyle) {
  if (!compStyle) {
    return "(gCS returned " + compStyle + ")";
  }
  return '"' + compStyle.fontWeight + '"';
}
onload = function() {
var docEl = document.body.firstChild.contentDocument.documentElement;
var docWin = document.body.firstChild.contentWindow;
docEl.style.fontWeight = "bold";
var out = "";
out += getFontWeight(getComputedStyle(docEl)) + " ";
out += getFontWeight(docWin.getComputedStyle(docEl)) + " ";
document.body.removeChild(document.body.firstChild);
document.body.textContent = out +
  getFontWeight(getComputedStyle(docEl)) + " " +
  getFontWeight(docWin.getComputedStyle(docEl));
}
</script>

IE10 Developer Preview and Opera Next 12.00 alpha output "700" "700" "700" "700".  Firefox 16.0a1 outputs "400" "700" "700" (gCS returned null).  Chrome 21 dev outputs "bold" "bold" "" (gCS returned undefined).  (There's no useful spec for CSSOM.)  So it looks like we do have computed styles for detached iframes, maybe, kind of?  I want to check with Boris what makes sense here.  The "400" sure doesn't make sense to me!  And I don't know why getComputedStyle on the iframe's own window returns null, but using the getComputedStyle on the actual window seems to work (at least in this trivial case).

Basically, anything computed style doesn't work for, we definitely don't want to be editable.  But I'm not sure what the right definition for that is.

    
    

    
  
Sounds like the plan is to disallow editor commands when there are selection endpoints that aren't descendants of the document.  I'm looking forward to this new world of sanity!
Whiteboard: [fuzzblocker:editor-assertions]

    
    

    
  
Why does this have anything to do with computed styles?

    
    

    
  
Because the reason for the failure is that GetCSSEquivalentToHTMLInlineStyleSet calls GetDefaultViewCSS to get the window, so that GetCSSInlinePropertyBase can call GetComputedStyle on it.  So if there were some way to get at the computed style, we could do that and have it still work perfectly fine.  Comment 8 suggests that the computed style does actually exist, so the idea would be to get it somehow without having to run through OwnerDoc()->GetWindow(), which fails in this case for whatever reason.  Or fix OwnerDoc()->GetWindow() so it returns a usable window here.

But basically everything boils down to our ability to get computed style.  As long as that can happen, there's no reason this shouldn't work.

    
    

    
  
Hmm, so nsComputedDOMStyle::GetStyleContextForElement is the lowest level thing which returns the computed style that I know of, and it reads the presshell from the document's docshell <http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#304>.  GetComputedStyle itself gets the presshell from the docshell stored on the window object <http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8153>.  Try calling GetStyleContextForElement, but if you don't have a presshell when you need one, I don't know of a way to read the computed style...

    
    

    
  
> but we do compute style for detached iframes.

When computing style on an element via getComputedStyle, we use the style set of the current document of the window that getComputedStyle was called on.  Except of course you're using inline style, so that comes from the element no matter what.  The spec issue on how this should behave in cross-window cases is still open.

The 400 comes from the fact that at that point the inline style change hadn't been processed yet, and we only flush on the window we're calling getComputedStyle on, not on the document of the element (possibly a bug).

The style computation process fundamentally requires a presshell.  Specifically, style data is allocated out of the presshell's arena, so no presshell means you just can't create those objects, period.

    
    

    
  
Funny, we have the exact same problem w.r.t. computed style in WebKit.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #13)
> The style computation process fundamentally requires a presshell. 
> Specifically, style data is allocated out of the presshell's arena, so no
> presshell means you just can't create those objects, period.

I eventually figured out how to get a computed style object directly from the element, but yes, it doesn't work without a presshell.  So what are the conditions under which we have a presshell?  In any other case, we should make most commands disabled.

(In reply to Ryosuke Niwa from comment #14)
> Funny, we have the exact same problem w.r.t. computed style in WebKit.

If we can agree on the cases where computed style is not available, we should make commands disabled if the selection is in such nodes.

    
    

    
  
Over on bug 671152, we're talking about making nodes non-selectable if they don't descend from window.document.  If we do that, this should be fixed too, and very thoroughly so.

    
    

    
  
> So what are the conditions under which we have a presshell? 

There's a presshell whenever the window's frameElement has a CSS box.  So that element must be in the DOM, it must not be in a display:none subtree, and the same needs to be true of the frameElements of everything on the window.parent chain (though crossing the chrome boundary too, which window.parent normally does not).

Note that this is a non-obvious implementation limitation that I'm not entirely happy enshrining in a spec.  In particular, the set of such cases differs in WebKit and Gecko, and will be different again in servo I bet.

> If we do that, this should be fixed too, and very thoroughly so.

No, see display:none above.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #17)
> There's a presshell whenever the window's frameElement has a CSS box.  So
> that element must be in the DOM, it must not be in a display:none subtree,
> and the same needs to be true of the frameElements of everything on the
> window.parent chain (though crossing the chrome boundary too, which
> window.parent normally does not).

data:text/html,<!DOCTYPE html>
<style>head { display: none; color: red }</style>
<script>
document.documentElement.textContent =
getComputedStyle(document.head).color
</script>

I get "rgb(255, 0, 0)" in Gecko.  How does that happen, given the head is display: none?

> Note that this is a non-obvious implementation limitation that I'm not
> entirely happy enshrining in a spec.  In particular, the set of such cases
> differs in WebKit and Gecko, and will be different again in servo I bet.

I think it's fair to say that nodes that don't descend from window.document aren't selectable, and that almost sidesteps the issue -- except maybe for display: none.

    
    

    
  
> How does that happen, given the head is display: none?

I think you misread what I wrote.  Try this:

  <iframe style="display: none" 
          src="data:text/html,<!DOCTYPE html>
               <script>
                 alert(getComputedStyle(document.head).color);
               </script>">
  </iframe>

and watch the resulting exception.

    
    

    
  


  
Enums -- hi-tech stuff!
Assignee: nobody → ayg
Status: NEW → ASSIGNED

        Attachment #641798 -
        Flags: review?(ehsan)

    
    

    
  


  
NS_NewComputedDOMStyle previously returned nsresult.  It had two failures paths: new failing (which is now impossible), and Init() failing.  Init() could fail if:

* It was passed a null pointer.  Changed to MOZ_ASSERT.
* Its nsIDOMElement* argument didn't QI to nsIContent*.  This was impossible and could have been a MOZ_ASSERT anyway, but I switched its argument to dom::Element so QI wasn't needed.
* do_GetAtom() failed when getting a pseudo-element.  Looking at the implementation, this doesn't seem to be possible.  Changed to use MOZ_ASSERT.
* aPresShell->GetPresContext() returned null.  Looking at the source code for nsPresShell, it looks like mPresContext is initialized at Init() and destroyed at Destroy().  Changed to use MOZ_ASSERT.  This is the only one I wasn't absolutely sure of, since it's not actually in the constructor/destructor and so it's possible to have an uninitialized object lying around.

This moves a bunch of code from lower in the file to higher, which fortunately shows up in the diff as a bunch of code being moved down lower.  I didn't actually change any of the stuff between the constructor and Init(), just moved stuff from Init() to the constructor and modified that, so don't mind the moved lines.

        Attachment #641800 -
        Flags: review?(bzbarsky)

    
    

    
  


  
Every caller was doing nothing but use the result to retrieve some computed style, so let's just fetch the computed style object directly.  This changes the assertion in the test-case to "Trying to compute style without PresShell".

        Attachment #641801 -
        Flags: review?(ehsan)

    
    

    
  
Try for the above series: https://tbpl.mozilla.org/?tree=Try&rev=c65f6378ef9b

It turns out they're all basically just cleanup -- they don't do anything to fix the bug.  AFAICT, bug 671152 will fix this.  If we make it impossible to put the selection outside window.document, then this test-case will fail at the selectAllChildren() step.  You can't modify it by calling getSelection() on the frame's window or document rather than the parent's, because that returns null on detached frames.

Basically, if a document's selection can't be in anything but a descendant of that document, and if a document with no presshell can't have a selection, there's no way to try running commands when the selection is in an element whose document doesn't have a presshell.

    
    

    
  
Comment on attachment 641800 [details] [diff] [review]
Patch part 2 -- Make nsComputedDOMStyle Init infallible and merge into constructor

>+  nsCOMPtr<dom::Element> element = do_QueryInterface(aElt);

As things stand, this can return null.  Both if aElt is null and if it's not.

Which is why there used to be those null-checks before and after the QI in NS_NewComputedDOMStyle.

So you need a null-check on "element" here.

r=me if you add that.

        Attachment #641800 -
        Flags: review?(bzbarsky) → review+

    
  

        Attachment #641798 -
        Flags: review?(ehsan) → review+

    
  

        Attachment #641801 -
        Flags: review?(ehsan) → review+

    
    

    
  
Comment on attachment 641803 [details] [diff] [review]
Patch part 4 -- Clean up nsHTMLCSSUtils::GetCSSInlinePropertyBase

Review of attachment 641803 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsHTMLCSSUtils.cpp
@@ +596,3 @@
>    }
> +
> +  // aStyleType == eSpecified

Can you MOZ_ASSERT this?  :-)

        Attachment #641803 -
        Flags: review?(ehsan) → review+

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #25)
> >+  nsCOMPtr<dom::Element> element = do_QueryInterface(aElt);
> 
> As things stand, this can return null.  Both if aElt is null and if it's not.
> 
> Which is why there used to be those null-checks before and after the QI in
> NS_NewComputedDOMStyle.
> 
> So you need a null-check on "element" here.
> 
> r=me if you add that.

Okay, done -- but really?  There's such a thing as an nsIDOMElement that's not a dom::Element?  Or do we just have to be paranoid because it's not a builtinclass?


https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf6519b3f43
https://hg.mozilla.org/integration/mozilla-inbound/rev/108d06ef7755
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcff1db63b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5629b973e4

Note to mergers: the bug isn't actually fixed, and it's only cleanup patches that have landed, so I'm not setting the target milestone yet.
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [fuzzblocker:editor-assertions] → [fuzzblocker:editor-assertions][Leave open]

    
    

    
  
> Or do we just have to be paranoid because it's not a builtinclass?

Precisely.  XPConnect will happily make up an nsIDOMElement for you.  We need to just fix that....

    
    

    
  
(In reply to Ed Morley [:edmorley] from comment #28)
> Push backed out for failing to build:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6ceeca8b4b73
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/670ad979c494

This was due to bug 771594, which changed the definition of nsCSSProps::LookupProperty to require a second param.  The call in my code therefore broke.  I made it use eEnabled, but it makes no difference for us because we should never be using properties that can be disabled.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7a036d6973
https://hg.mozilla.org/integration/mozilla-inbound/rev/e35000ce9795
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0c13733a84
https://hg.mozilla.org/integration/mozilla-inbound/rev/607d417f36e8
Depends on: 776879
Assignee: ayg → nobody
Status: ASSIGNED → NEW

    
  
Depends on: 671152

    
    

    
  
Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority and severity.


If you have reason to believe this is wrong, please write a comment and ni :jstutte.


Severity: normal → S4
Priority: -- → P5

    
    

    
  
Given that this is a fuzzblocker, it should probably be higher than S4 severity.


Severity: S4 → --
Flags: needinfo?(jstutte)
Priority: P5 → --

    
    

    
  
Is this still a fuzzblocker? The flag has been set 11 years ago and we do not receive the fuzzblocker nags on it.


Flags: needinfo?(jstutte) → needinfo?(choller)

    
    

    
  
According to comment 31, this should be fixed. I'm going to close it as such. If that's wrong, please reopen.


Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(choller)
Resolution: --- → FIXED

    
    

    
  
(In reply to Christian Holler (:decoder) from comment #35)




According to comment 31, this should be fixed. I'm going to close it as such. If that's wrong, please reopen.




Ah thanks, did miss that hint.


Assignee: nobody → ayg

    
  
Whiteboard: [fuzzblocker:editor-assertions][Leave open] → [fuzzblocker:editor-assertions]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: