Closed Bug 1355819 Opened 7 years ago Closed 7 years ago

Editing a selector doesn't work: "Protocol error (unknownError): sheet.ownerNode is null"

Categories

(DevTools :: Inspector: Rules, defect, P1)

defect

Tracking

(firefox54 wontfix, firefox55 verified)

VERIFIED FIXED
Firefox 55
Tracking Status
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: ntim, Assigned: hemantsingh1612, Mentored)

Details

Attachments

(3 files)

STR:

- Go to http://www.paradegroup.com/portal/register/select
- Select .register-blank in the inspector
- Try to edit ".register-type-container .register-blank"

AR:

Selector does not change
Protocol error (unknownError): sheet.ownerNode is null  devtools/client/inspector/shared/utils.js:157

ER:

Selector changes
Thanks for filing. Here's the full stack trace:

getDocument@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1009:7
StyleRuleActor<._addNewSelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1347:22
_run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
modifySelector2@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:1453:27
handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1082:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1769:15
receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:761:7
It looks like this particular stylesheet doesn't have an ownerNode, but it has a parentStyleSheet. So that means the stylesheet was loaded via an @import directive, rather than via a <link> node.
This is an embarrassing problem that basically breaks a functionality of the rule-view, so I would like to triage this as a P1.
Is someone interested in taking this on? It's not a particularly hard issue to fix I believe. We need to make the getDocument function safer in devtools\server\actors\styles.js.
If we pass a sheet object that has no ownerNode, it shouldn't fail, it should just check if the sheet has a parentStyleSheet, and get the ownerNode from there instead (there might be a chain of parentStyleSheets to walk up I believe).

If this fix is enough, then the only other thing to do is adding an automated test.
The easiest thing to do here is copy/paste one of the devtools\client\inspector\rules\test\browser_rules_edit-selector_*.js files to create a new one that basically replicates the problem described in comment 0.

I can mentor this bug if someone is interested in fixing it.
Make sure you read the instructions on how to get the code and run it first: https://wiki.mozilla.org/DevTools/Hacking
Mentor: pbrosset
Priority: -- → P1
Well I am ready for this opportunity. It's quite interesting to work upon.
Just saw this bug on twitter! Let me know if this one is still up for grabs. 
Trying to get started with running the code, but what does AR and ER mean?
Flags: needinfo?(pbrosset)
(In reply to Atsushi Yamamoto [:atsushi] from comment #5)
> Just saw this bug on twitter! Let me know if this one is still up for grabs. 
> Trying to get started with running the code, but what does AR and ER mean?
Cool, thanks for your interest, but Hemant Singh Patwal already said they were going to work on this. Hemant: still interested?

Anyway, sorry about the confusion with AR/ER: AR means actual result, ER expected result.
Flags: needinfo?(pbrosset) → needinfo?(hemantsingh1612)
(In reply to Patrick Brosset <:pbro> from comment #6)
> (In reply to Atsushi Yamamoto [:atsushi] from comment #5)
> > Just saw this bug on twitter! Let me know if this one is still up for grabs. 
> > Trying to get started with running the code, but what does AR and ER mean?
> Cool, thanks for your interest, but Hemant Singh Patwal already said they
> were going to work on this. Hemant: still interested?
> 
> Anyway, sorry about the confusion with AR/ER: AR means actual result, ER
> expected result.

Sure.
Flags: needinfo?(hemantsingh1612)
Hemant: any updates on this? Ideally I'd like this to be fixed rather soon, so let me know where you are with this and if you need more help please.
Flags: needinfo?(hemantsingh1612)
In my case if I remove the selector .register-blank I get an input field.And here it is mentioned that selector does not change!
I am using Firefox 54.0a2 (2017-04-18)developer edition/
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #9)
> In my case if I remove the selector .register-blank I get an input field.And
> here it is mentioned that selector does not change!
> I am using Firefox 54.0a2 (2017-04-18)developer edition/
Yes, indeed, the input field does appear, but the problem described in comment 0 comes at the next step: once you have the input field, try to enter a different selector, maybe just "div" for instance, and press <ENTER> to submit that change. 
You will see that the new selector is *not* taken into account, and the old one is reverted.
I that in my case everything is working fine!!!
Flags: needinfo?(hemantsingh1612)
(In reply to Patrick Brosset <:pbro> from comment #10)
> (In reply to Hemant Singh Patwal [:hhhh1612] from comment #9)
> > In my case if I remove the selector .register-blank I get an input field.And
> > here it is mentioned that selector does not change!
> > I am using Firefox 54.0a2 (2017-04-18)developer edition/
> Yes, indeed, the input field does appear, but the problem described in
> comment 0 comes at the next step: once you have the input field, try to
> enter a different selector, maybe just "div" for instance, and press <ENTER>
> to submit that change. 
> You will see that the new selector is *not* taken into account, and the old
> one is reverted.

But in my case that's not true i think..
I am able to make changes to other selectors also,without reverting the old one.
Flags: needinfo?(pbrosset)
Here are the steps from comment 0, in FF54 (devedition).
As you can see, after I press submit, the selector is back to what it was before my change, and an error appears in the Browser Console.

I hope this helps.
If you can't reproduce this bug, it's going to make it hard for you to fix it :)
Flags: needinfo?(pbrosset)
Ahh! I see.
Sorry, I mistook it other way
So getDocument function in devtools\server\actors\styles.js ownerNode is to be replaced by parentStylesheet.
Hi Hermant, it's been a few weeks. Are you still interested in fixing this?
Assignee: nobody → hemantsingh1612
Status: NEW → ASSIGNED
Flags: needinfo?(hemantsingh1612)
Flags: needinfo?(hemantsingh1612)
Comment on attachment 8867363 [details]
Bug 1355819 - Make the getDocument function safer in devtools\server\actors\styles.js.If passed object has no ownerNode get it from parentStyleSheet instead.

https://reviewboard.mozilla.org/r/138890/#review142450

That's a great start. Unfortunately, it still fail. See my 2nd comment below.

::: commit-message-1ec1d:1
(Diff revision 2)
> +Bug 1355819 - Editing a selector doesn't work: "Protocol error (unknownError): sheet.ownerNode is null". r?pbro

Could you please rephrase this commit message? Instead of re-iterating the bug title, it would be more useful to write a short summary of your fix.

::: devtools/server/actors/styles.js:1012
(Diff revision 2)
>    },
>  
>    getDocument: function (sheet) {
>      let document;
>  
>      if (sheet.ownerNode instanceof Ci.nsIDOMHTMLDocument) {

This is still going to fail.
The error message described in comment 0 is "sheet.ownerNode is null". This means that we first have to test if sheet.ownerNode even exist before testing its type.
Attachment #8867363 - Flags: review?(pbrosset)
Comment on attachment 8867363 [details]
Bug 1355819 - Make the getDocument function safer in devtools\server\actors\styles.js.If passed object has no ownerNode get it from parentStyleSheet instead.

https://reviewboard.mozilla.org/r/138890/#review144114

::: devtools/server/actors/styles.js:1012
(Diff revision 3)
>    },
>  
>    getDocument: function (sheet) {
>      let document;
>  
> +    if (typeof sheet.ownerNode != "undefined") {

Did this work when you tested it? I'm confused here. We said that the stylesheet that's failing in this bug doesn't have an ownerNode. So that means the function will now return undefined, and I don't think this can work, because we need a document to make the selector change.

I think the logic we want is something like this:

1. Check if sheet.ownerNode exists
1a. If it doesn't check if sheet.parentStyleSheet exists and walk up the parentStyleSheets until you find one that has an ownerNode and use that.
1b. If sheet.ownerNode did exist in the first place, just use that
2. Just do the same as before.

Does that make sense?
Make sure you build your changes and test with the steps described in comment 0 to see if your fix does work.

It would be great if you could create a test for this too. Let me know if you need help for this.
Attachment #8867363 - Flags: review?(pbrosset)
I want to try this:
 getDocument: function (sheet) {
    let document;

    if (sheet.ownerNode instanceof Ci.nsIDOMHTMLDocument) {
      document = sheet.ownerNode;
    } else {
      document = 'ownerNode' in sheet.parentStyleSheet ? sheet.parentStyleSheet.ownerNode.ownerDocument : this.getDocument(sheet.parentStyleSheet)
    }

  return document;
  }

I am not sure if getDocument is properly called here.
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #21)
> I want to try this:
>  getDocument: function (sheet) {
>     let document;
> 
>     if (sheet.ownerNode instanceof Ci.nsIDOMHTMLDocument) {
>       document = sheet.ownerNode;
>     } else {
>       document = 'ownerNode' in sheet.parentStyleSheet ?
> sheet.parentStyleSheet.ownerNode.ownerDocument :
> this.getDocument(sheet.parentStyleSheet)
>     }
> 
>   return document;
>   }
Shouldn't the else part be:

else if (sheet.parentStylesheet) {
  document = this.getDocument(sheet.parentStyleSheet);
}

?

Other than that, it seems like you could get rid of that `document` variable that's stored in memory, especially if you're calling getDocument recursively.

So you could end up with something like this:

getDocument(sheet) {
  if (sheet.ownerNode) {
    return sheet.ownerNode instanceof Ci.nsIDOMHTMLDocument ? 
           sheet.ownerNode : ownerNode.ownerDocument;
  }
  if (sheet.parentStyleSheet.ownerNode) {
    return this.getDocument(sheet.parentStyleSheet);
  }
}


> I am not sure if getDocument is properly called here.

If you're not sure, add a console.log(this) and run your changes, and see if `this` contains getDocument, or simply test your changes using steps from comment 0. If you need help setting up your build environment, you can ask in Mozilla's IRC #developers channel.
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #21)
> I am not sure if getDocument is properly called here.

I checked quickly the code, and it seems like this.getDocument should work.

But while doing so, I've noticed a potential code path that could be simplified:

https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/styles.js#1312

It seems like there's some code that walks up parentStyleSheets until an ownerRule is found, but it might not be necessary if getDocument also walks up parentStyleSheets.

Patrick, what do you think?
Flags: needinfo?(pbrosset)
Comment on attachment 8867363 [details]
Bug 1355819 - Make the getDocument function safer in devtools\server\actors\styles.js.If passed object has no ownerNode get it from parentStyleSheet instead.

https://reviewboard.mozilla.org/r/138890/#review144468

This looks great. I like the recursive call to getDocument. Makes a lot of sense.
I can't quite R+ this however without a test though. You can do this in a separate commit though. Up to you how you want to do it, but it needs to be done in the same bug.
Here's some help about how to create this test:

1. Create a new file in \devtools\client\inspector\rules\test\
2. Name it browser_rules_edit-selector_12.js
3. Add an entry for it in \devtools\client\inspector\rules\test\browser.ini, just like it's done for other tests
4. I suggest copy/pasting the content of browser_rules_edit-selector_11.js into your new file, just to get some structure for your new test
5. You'll need some support files for your test. I suggest creating a test HTML page, with a stylesheet file that contains an @import rule that points to yet another stylesheet file. The goal is to reproduce the test case described in comment 0.
6. Add all of these support files in \devtools\client\inspector\rules\test\browser.ini. You'll see there's a section for this in this config file
7. Write your test code to open this HTML page, open the inspector, simulate a change of selector on the right node, etc...
8. Test that your test *fails* first, by removing your fix. This way you can be sure that you have the right test case.
9. Put your fix back in, run the test again and check that it *passes*.

You can run your new test with ./mach test devtools/client/inspector/rules/test/browser_rules_edit-selector_12.js
I also suggest running all the other tests locally to make sure your change didn't break anything unexpected: ./mach test devtools/client/inspector/rules/test/

::: devtools/server/actors/styles.js:1014
(Diff revision 5)
>    getDocument: function (sheet) {
> -    let document;
> -
> -    if (sheet.ownerNode instanceof Ci.nsIDOMHTMLDocument) {
> -      document = sheet.ownerNode;
> -    } else {
> +    if (sheet.ownerNode) {
> +      return sheet.ownerNode instanceof Ci.nsIDOMHTMLDocument ?
> +             sheet.ownerNode : sheet.ownerNode.ownerDocument;
> +   }
> +    else if (sheet.parentStyleSheet) {

nit: please move the else if on the same line as the closing } of the if:

```javascript
if (sheet.ownerNode) {
  return sheet.ownerNode instanceof Ci.nsIDOMHTMLDocument ?
         sheet.ownerNode : sheet.ownerNode.ownerDocument;
} else if (sheet.parentStyleSheet) {
  return this.getDocument(sheet.parentStyleSheet);
}
```

::: devtools/server/actors/styles.js
(Diff revision 5)
> -    if (sheet.ownerNode instanceof Ci.nsIDOMHTMLDocument) {
> -      document = sheet.ownerNode;
> -    } else {
> -      document = sheet.ownerNode.ownerDocument;
> +             sheet.ownerNode : sheet.ownerNode.ownerDocument;
> +   }
> +    else if (sheet.parentStyleSheet) {
> +      return this.getDocument(sheet.parentStyleSheet);
>      }
> -

So, this function has an if, an else if, but no catch all else.
This means that it can potentially return undefined in a non explicit manner if the sheet object has neither an ownerNode nor a parentStyleSheet.

This is nothing new though, the old code didn't work in this case either.
But I think we ought to make all cases explicit. So I would suggest to throw at the end of getDocument, like this:

```javascript
getDocument: function (sheet) {
  if (sheet.ownerNode) {
    ...
  } else if (sheet.parentStyleSheet) {
    ...
  }
  
  throw(new Error("Failed trying to get the document of an invalid stylesheet"));
}
```
Attachment #8867363 - Flags: review?(pbrosset)
(In reply to Tim Nguyen :ntim (not accepting requests until 1st June) from comment #23)
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/styles.
> js#1312
> 
> It seems like there's some code that walks up parentStyleSheets until an
> ownerRule is found, but it might not be necessary if getDocument also walks
> up parentStyleSheets.
> 
> Patrick, what do you think?
You're right, it looks like we could clean this up too.
I'm not sure what the relationship is between sheet.parentStyleSheet and shee.ownerRule.parentStyleSheet.
We should check if sheet.parentStyleSheet can exist when sheet.ownerRule doesn't.
And we should check if sheet.parentStyleSheet === sheet.ownerRule.parentStyleSheet.
Flags: needinfo?(pbrosset)
Comment on attachment 8867363 [details]
Bug 1355819 - Make the getDocument function safer in devtools\server\actors\styles.js.If passed object has no ownerNode get it from parentStyleSheet instead.

https://reviewboard.mozilla.org/r/138890/#review145124

Looks great. Thanks.
I'm uploading a test for this fix. I'll push it to the CI environment to check everything still runs well, and then we should be good to land this.
Attachment #8867363 - Flags: review?(pbrosset) → review+
The TRY build is looking good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a28cf80cac3a50f03e46b13b9d388f4747f5d78a&group_state=expanded
As soon as the test gets reviewed, we can land your fix Hemant! Thanks for this.
Comment on attachment 8870021 [details]
Bug 1355819 - New regression test for editing selectors of @import'd rules;

https://reviewboard.mozilla.org/r/141542/#review145148

LGTM, thanks!

::: devtools/client/inspector/rules/test/doc_edit_imported_selector.html:1
(Diff revision 1)
> +<!DOCTYPE html>

We are not super consistent about it, but let's add a license header
Attachment #8870021 - Flags: review?(jdescottes) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5df14ee6bba6
Make the getDocument function safer in devtools\server\actors\styles.js.If passed object has no ownerNode get it from parentStyleSheet instead. r=pbro
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b306da6fbb4
New regression test for editing selectors of @import'd rules; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/5df14ee6bba6
https://hg.mozilla.org/mozilla-central/rev/5b306da6fbb4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Mark 54 won't fix as this is late for Beta54.
https://hg.mozilla.org/projects/cedar/rev/5df14ee6bba6e71dae8058a6ab1a50c314a48a8e
Bug 1355819 - Make the getDocument function safer in devtools\server\actors\styles.js.If passed object has no ownerNode get it from parentStyleSheet instead. r=pbro

https://hg.mozilla.org/projects/cedar/rev/5b306da6fbb486d5328cb0af4c0c4dcf1ce91bcd
Bug 1355819 - New regression test for editing selectors of @import'd rules; r=jdescottes
I have reproduced this bug with Nightly 55.0a1 (2017-04-12) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 55.0a1 !

Build   ID    20170605030204
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170531]
I have reproduced this Bug with Nightly 55.0a1 (2017-04-12) (64-bit) on Ubuntu 16.04 LTS!

The bug's fix is verified with latest Beta!

Build ID 	20170706155135
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
QA Whiteboard: [bugday-20170705]
As per Comment 39 & Comment 40, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: