Closed Bug 816953 Opened 11 years ago Closed 10 years ago

Intermittent browser_inspector_markup_edit.js | uncaught JS exception - TypeError: domRules is null at resource:///modules/devtools/CssLogic.jsm:632

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: jwalker, Assigned: miker)

References

Details

(Keywords: intermittent-failure, Whiteboard: [has-patch])

Attachments

(1 file)

Wait for "markupmutation" event before calling setTimeout
1.33 KB, patch
Details | Diff | Splinter Review 
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: domRules is null at resource:///modules/devtools/CssLogic.jsm:632 

https://tbpl.mozilla.org/?tree=Fx-Team&rev=c77b869c2025

https://tbpl.mozilla.org/php/getParsedLog.php?id=17481911&tree=Fx-Team

TEST-INFO | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | START Remove an element with the delete key
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | Node 18 should exist.
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | Node 18 should not exist.
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | Should be able to undo.
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | Node 18 should exist.
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | Should be able to redo.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: domRules is null at resource:///modules/devtools/CssLogic.jsm:632
Stack trace:
    JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 1066
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | Console message: [JavaScript Warning: "XUL box for span element contained an inline #text child, forcing all its children to be wrapped in a block." {file: "chrome://browser/content/devtools/layoutview/view.xhtml" line: 0}]
TEST-PASS | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | Node 18 should not exist.
TEST-INFO | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | END Remove an element with the delete key
TEST-INFO | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | Console message: [JavaScript Error: "TypeError: domRules is null" {file: "resource:///modules/devtools/CssLogic.jsm" line: 632}]
INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/markupview/test/browser_inspector_markup_edit.js | finished in 1638ms
Keywords: intermittent-failure
Whiteboard: [orange]
(TBPL & OrangeFactor just require the intermittent-failure keyword in order for bug suggestions to work; [orange] is depreciated - dev.platform post coming shortly)
Summary: TEST-UNEXPECTED-FAIL | .../devtools/markupview/test/browser_inspector_markup_edit.js | ... domRules is null at CssLogic.jsm:632 → Intermittent browser_inspector_markup_edit.js | uncaught JS exception - TypeError: domRules is null at resource:///modules/devtools/CssLogic.jsm:632
Whiteboard: [orange]
Blocks: 788977
Mike, do you see anything obvious here?

The failing code is related to your recent changes in browser_inspector_markup_edit.js:

https://github.com/joewalker/devtools-window/commit/354ad6fc925f6897cfd677a8f826c7226d43461d
https://github.com/joewalker/devtools-window/commit/2a87fcf1d52aac8cb189c68ac4802e7e66f92458
Flags: needinfo?(mratcliffe)
I think this is line 369 of https://github.com/joewalker/devtools-window/commit/354ad6fc925f6897cfd677a8f826c7226d43461d#L0L371

I had to add executeSoon here because the undo/redo manager takes time before undo will work properly. Obviously, some kind of event would work better.
Flags: needinfo?(mratcliffe)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #19)
> I think this is line 369 of
> https://github.com/joewalker/devtools-window/commit/
> 354ad6fc925f6897cfd677a8f826c7226d43461d#L0L371
> 
> I had to add executeSoon here because the undo/redo manager takes time
> before undo will work properly. Obviously, some kind of event would work
> better.

What is asynchronous here? Why do we need executeSoon in the first place?
Blocks: 820315
Making a change, undoing and redoing in rapid succession can currently cause this problem (the undo/redo manager is not finished updating itself). If the undo/redo manager fired an event to signify that it is ready we could overcome this.

What is asynchronous here? Why do we need executeSoon in the first place?

I have no idea which parts of the undo/redo manager are async. All I know is that the process I describe throws that error if you don't use executeSoon.
I think I have a fix for that (part of bug 814889).
Depends on: 814889
Assignee: nobody → mratcliffe
Unfortunately this is caused by the mutation observer.

We call Highlighter.jsm::updateInfobar() using Selection.jsm:87 (this.emit("attribute-changed"))

Because the MutationObserver gathers *batches* of events and then periodically calls the _onMutations listener (only then emitting "attribute-changed") we have the async issue.

We can wait for markupmutation before calling executeSoon and it should work.
No new reports for 3 weeks?

Anyhow, waiting for "markupmutation" before setTimeout can't hurt. This is what we always did before calling undo and redo ... we never had issues at that point but only used setTimeout after redo.
Attachment #703236 - Flags: review?(paul)
Whiteboard: [has-patch]
Status: NEW → ASSIGNED
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #42)
> Created attachment 703236 [details] [diff] [review]
> Wait for "markupmutation" event before calling setTimeout
> 
> No new reports for 3 weeks?

I made a lot of changes in this test.

> Anyhow, waiting for "markupmutation" before setTimeout can't hurt. This is
> what we always did before calling undo and redo ... we never had issues at
> that point but only used setTimeout after redo.

Are you sure all the tests cause a mutation?
Why do we need to be in a second event loop to call test.after?
I probably fixed this bug while fixing bug 827990 or bug 814889.

Let's re-open if we run into new oranges.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
Attachment #703236 - Flags: review?(paul)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.