746197 - Style Inspector: TypeError: domRules is null

Status: RESOLVED WORKSFORME

(DevTools :: Inspector, defect, P1)

Description

[Paul Rouget [:paul]]

Error: TypeError: domRules is null
Source File: resource:///modules/devtools/CssRuleView.jsm
Line: 190

To reproduce:

* go to http://duckduckgo.com/
* bottom right corner, right click on "Track", then "Inspect Element"
* Open the Rule View

Comment 1

[Paul Rouget [:paul]]

(I meant bottom left)

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Fix missing tests

This fixes the issue but we still need to create tests for the rule view, property view, highlighter and layout panel.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Quickly hacked together test file

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

This patch is green on try:
https://tbpl.mozilla.org/?tree=Try&rev=6efba986cef3

Comment 6

[Paul Rouget [:paul]]

Comment on attachment 624403 [details] [diff] [review]
Patch

Happy to finally see some mutation observers in our code :)
That seems like the right approach here.

Can we be more generic though? This feel too much specific.

The problem described here is that when a node is deleted, our code fails.
Our code fails because that most of the inspector code assume that the DOM is static.

Do you think we could design a more global solution to this problem now?
It probably doesn't have to cover all the cases, but for example, since we have the observer, we can start updating other things when some DOM mutations happen (like invalidating the breadcrumbs, updating the infobar the HTML panel).

I don't the cost of listening more types of mutations though.

Also, in your code, addNodeRemovalObserver will update some tools (infobar) AND fire a notification. I think your observer should either fire a global notification, or update all the tools, but not do both. That can be confusing.

Maybe each component (layout view, breadcrumbs, HTML tree, highlighter, infobar, …) should have an "invalidate" method that would be called from the Inspector Mutation observer.

Or maybe, each component should be responsible of listening to mutations.

I think we need to think hard about what is the right design here.

But maybe before working on the "big thing", we want to temporally fix this specific case.

I think Dave should be the reviewer here.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Paul Rouget [:paul] from comment #6)
> Comment on attachment 624403 [details] [diff] [review]
> Patch
> 
> Happy to finally see some mutation observers in our code :)
> That seems like the right approach here.
> 
> Can we be more generic though? This feel too specific.
> 

This is true, now we have these mutation listeners we should be updating our tools where possible.

> The problem described here is that when a node is deleted, our code fails.
> Our code fails because that most of the inspector code assume that the DOM
> is static.
> 
> Do you think we could design a more global solution to this problem now?
> It probably doesn't have to cover all the cases, but for example, since we
> have the observer, we can start updating other things when some DOM
> mutations happen (like invalidating the breadcrumbs, updating the infobar
> the HTML panel).
> 

Yup, a good idea. Rather than re-implement the same code repeatedly in our tools we should send a notification specific to a particular case, e.g. NODE_REMOVED when a node (or a node's ancestor) is removed from the document, NODE_MOVED when a node (or a node's ancestor) has been moved to a new node, ATTRIBUTE_CHANGED when an attribute is changed and any others that we could trace and any others we can think of that may be relevant.

> I don't like the cost of listening more types of mutations though.
> 

Because we receive the events in batches the impact on performance should be minimal, we will only find out about the performance impact if we use them. When I tried listening for all mutations I didn't notice any slow down at all.

> Also, in your code, addNodeRemovalObserver will update some tools (infobar)
> AND fire a notification. I think your observer should either fire a global
> notification, or update all the tools, but not do both. That can be
> confusing.
> 

Agreed, you got me on that one. In fact I would say that the inspector should fire specific notifications as I mentioned above. Otherwise each tool would have to reimplement checks e.g. Has the node (or it's ancestor) been removed, has a selected node's or it's ancestor been moved etc. We know the specific cases that we are interested in so the inspector could deal with raising notifications just when our specific needs are met.

> Maybe each component (layout view, breadcrumbs, HTML tree, highlighter,
> infobar, …) should have an "invalidate" method that would be called from the
> Inspector Mutation observer.
> 
> Or maybe, each component should be responsible of listening to mutations.
> 

If each tool listened for mutations we would have a lot of duplication of code, they would be better listening for the specific notifications I mention above and acting on them.

> I think we need to think hard about what is the right design here.
> 

I think that our tools listening for the inspector's notifications makes the most sense. In my view the inspector should be the center of communications when it comes to these things. I am open to better ideas if you have any.

> But maybe before working on the "big thing", we want to temporally fix this
> specific case.
> 

I should replace the code for updating the infobar with a notification listener for this case.

> I think Dave should be the reviewer here.

Yup, if he agrees with my comments above I will be happy to log a new bug for this ... it is not a big job.

Comment 8

[Paul Rouget [:paul]]

Bug 761250 might be related.

Comment 9

[Paul Rouget [:paul]]

Bug 719552 is related.

Comment 10

[Paul Rouget [:paul]]

Sorry, I meant bug 758422.

Comment 11

[Paul Rouget [:paul]]

In the meantime, you might want to use the method in this bug to avoid the exception: bug 769299

Comment 12

[Dave Camp (:dcamp)]

Attachment: alternative patch

This patch takes a different approach:

Rather than make every tool resilient to disconnected/dead nodes, this adds some observers in the breadcrumbs.  The breadcrumbs will select a new node when the currently-selected node is removed from the document.  So the highlighted node should be always be valid in the tools.

As a side effect this updates the breadcrumbs and infobar when ids and classes change, but it is Not a generic change-management mechanism for the inspector.  I agree that we should put thought in to how we design that, but don't think this fix should wait on that.

Comment 13

[Paul Rouget [:paul]]

Comment on attachment 653531 [details] [diff] [review]
alternative patch

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

I'm ok with this approach.

How does this behaves when the source of an inner iframe has changed?

And FYI, we have a helper for `nodeDisconnected`: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/LayoutHelpers.jsm#295

Comment 14

[Paul Rouget [:paul]]

Attachment: manual test: part 1

Comment 15

[Paul Rouget [:paul]]

Attachment: manual test: part 2

Comment 16

[Paul Rouget [:paul]]

Attachment: manual test: part 3

Comment 17

[Paul Rouget [:paul]]

I wrote this little manual test to experiment with this patch (sorry, not a mochitest).

Load PageA.html. Follow the instruction.

We see that:
- the first child is not updated if a node is appended (2).
- test 12 is broken

The changes in the iframe are correctly handled.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 624403 [details] [diff] [review]
Patch

See Paul's comments above

Comment 19

[Paul Rouget [:paul]]

Just a note (not saying we should use this, but it might be useful to track new iframes): we can be notified every time a document is created via the 'document-element-inserted' message (see https://developer.mozilla.org/en-US/docs/Observer_Notifications#Documents).

Comment 20

[Paul Rouget [:paul]]

Original bug is fixed by the new selection mechanism. But the manual test might not pass. I'll file a bug for these specific tests.

Bug triage, filter on PINKISBEAUTIFUL