Closed Bug 500365 Opened 15 years ago Closed 15 years ago

removeProperty fails if ownerNode is null

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
1.9.1 Branch
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: johnjbarton, Unassigned)

Details

Original report is http://code.google.com/p/fbug/issues/detail?id=1894
 Issue 1894:  	 CSS-edit removes declaration if page with specific javascript.

The report has a reproducible but complicated test case:
http://wesp.snt.utwente.nl/~alpha/firebug_test.htm

1. Install Firebug 1.5a6, http://getfirebug.com/releases/firebug/1.5X
2. load http://wesp.snt.utwente.nl/~alpha/firebug_test.htm
3. Open Firebug F12, enable Script and Net panel. Do *NOT* enable Console.
4. reload.
5. Inspect the page, click on C.S.V. Alpha 
6. In the Style side panel of HTML panel, click on .componentheading's color value ("#666666"). An inline editor will appear.
7. Type in 'red'.
8a. Wait, (setTimeout)
8b. click outside Firebug, (onBlur)
8c. hit Return key (key).

You will pass through 
 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSStyleRule.cpp#1050 
you will get an exception [nsIDOMCSSStyleDeclaration.removeProperty]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)", 
and the edit will fail (most of the time).

After the fail you have to reload, the DOM is damaged.

Occurs on FF 3.5RC2 and FF 3.6 trunk from maybe 6 weeks ago.

At the point of failure, the declarations has
style.parentRule.parentStyleSheet.ownerNode == null.

I loaded the page a few times and looked at that declaration's ownerNode. With the Firebug console disabled, the ownerNode is usually null; with the Console enabled, the node is usually 'link'. (Right click on the declaration, Inspect in DOM Tab, then look for "get parentRule" etc.

So my guess is that the DOM load is failing in general to set ownerNode correctly. When Firebug's Console is enabled, then Firebug is injecting code right at the first JS compile on the page, shifting the workload and letting the correct value be set.

The test case is quite complicated but so far I have not been able to find a smaller case.
I have other reports of this problem; it does not happen on FF3.0; I guess it could be an indication of a problem outside of editing in Firebug.
Whiteboard: [firebug-p1]
Oh, this is going through the style panel?  That's just using APIs known to be broken, assuming it works the way it does in DOM Inspector.  In particular, editing styles that come from clone sheets using that panel will give you all sorts of weirdness.  And in this case, I bet the difference is that you're picking up the clone of the preloaded sheet.

More generally, in that panel if you're looking at a rule you are NOT guaranteed that that rule is in the cssRules of the sheet it claims to be in.  So DOM manipulation of the rules is just busted.  We have existing bugs on this; probably filed on DOM Inspector.
To work around this, as soon as someone wants to use the style panel you need to walk through all sheets attached to the document (recursing into imported sheets) and get the first rule from the cssRules of each one....
(In reply to comment #2)
.... 
> So DOM manipulation of the rules is just busted.  We have existing bugs on
> this; probably filed on DOM Inspector.

Something does not add up, since the CSS editor in Firebug is easily the most widely used Firebug feature. Surely we would have hit this before now.
(In reply to comment #3)
> To work around this, as soon as someone wants to use the style panel you need
> to walk through all sheets attached to the document (recursing into imported
> sheets) and get the first rule from the cssRules of each one....

You mean just access the first rule?
> Surely we would have hit this before now.

Cloned sheets are not that common in webpages.  Until we did speculative sheet preloading you needed a web page that linked to a sheet twice.  Or one that used a chrome stylesheet, of course...

> You mean just access the first rule?

Yes.  Or any rule, really.  The point is to force unique per-sheet rules.  Cloned sheets share their rules, and the backdoor API the style panel uses to get at the rules doesn't force them to stop doing that...
Thanks Boris, at least in one quick test the problem is solved using code like:
 if (!this.context.forcedUniqueStyleSheets)
            {
                if (this.context.loaded)  // keep forcing until we are loaded
                    this.context.forcedUniqueStyleSheets = true;

                iterateWindows(this.context.window, bind(function(win)
                {
                    var doc = win.document;
                    var sheets = doc.styleSheets;
                    for(var i = 0; i < sheets.length; i++)
                    {
                        var rules = sheets[i].cssRules;
                        if (rules.length > 0)
                            var touch = rules[0];
                        if (touch)
                            FBTrace.sysout("css.show() touch "+typeof(touch)+" in "+win.location);
                    }
                }, this));
            }
For production use, you'd want to also walk into imported sheets, but that's the general idea, yes...

And now that I've had a chance to look for the original bug, marking duplicate.  Note also bug 384327 (which is pretty much an exact duplicate).
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
Sometimes the line
    var rules = sheets[i].cssRules;
gives an exception:
   A parameter or an operation is not supported by the underlying object
   NS_ERROR_DOM_INVALID_ACCESS_ERR
I wrapped it in a try block and ignored it.
You'll get that exception if you try to read a sheet before we've actually parsed it.
Touching the document.styleSheets as Boris suggests was implemented in Firebug 1.4 and works great, thanks Boris!
Resolution: DUPLICATE → WORKSFORME
Whiteboard: [firebug-p1]
Just to double-check, you did make sure to walk into imported sheets, right?
Well I have code like this:
                for (var i = 0; i < sheet.cssRules.length; ++i)
                {
                    var rule = sheet.cssRules[i];
                    if (rule instanceof CSSImportRule)
                        addSheet(rule.styleSheet);
                }
But if you know of a test case I can verify it easily.
Yeah, that should do the trick.
You need to log in before you can comment on or make changes to this bug.