Re-enable the browser_rules_pseudo-element_02.js with e10s

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

(Blocks 1 bug)

unspecified
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 fixed, firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
This test is currently disabled when tests are run with e10s and as such, is part of this list of tests we want to re-enable: https://docs.google.com/spreadsheets/d/10UeyRoiWV2HjkWwAU51HXyXAV7YLi4BjDm55mr5Xv6c/edit#gid=1777180571
(Assignee)

Comment 1

3 years ago
I have a pretty simple fix for this, but while investigating, I realized that this test was generating an exception while ending. This is bad because checking this fix in like this would introduce exceptions in the logs.
I don't believe it would fail the test though because this comes from an exception being logged in an handled promise rejection.
But bad nonetheless.

So, as I started investigating this, I found out that the code doesn't wait for one specific part of the async rule-view refresh process (when you select a new node).
This, therefore, causes exceptions when selecting a new element right before a test ends because, typically, devtools has been destroyed before the request/response can be handled.

The specific request is the one made by RuleEditors to retrieve original source links (which is useful when there are source maps).
Making the rule-view code wait for this to be done before signaling the inspector-panel that it is done refreshing gets rid of a this exception (which most probably occurs in a lot of tests).
(Assignee)

Updated

3 years ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Summary: Re-enable the browser_rules_content_02.js with e10s → Re-enable the browser_rules_pseudo-element_02.js with e10s
(Assignee)

Comment 3

3 years ago
I got confused when naming this bug and commit message, this isn't about browser_rules_content_02.js, but about browser_rules_pseudo-element_02.js
I'll rename the commit before pushing, for now, it doesn't matter.
Blocks: e10s-tests
tracking-e10s: --- → +
Comment on attachment 8713191 [details]
MozReview Request: Bug 1243736 - Enable browser_rules_content_02.js with e10s and make rule-view wait for updateSourceLink; r=bgrins

https://reviewboard.mozilla.org/r/32767/#review29833

Looks fine to me - don't forget to update the commit message as in Comment 3

::: devtools/client/inspector/rules/rules.js:1163
(Diff revision 1)
> +        let onEditorReady = promise.defer();

Doesn't 'once' already return a Promise?  If so could you do:

editorReadyPromises.push(rule.editor.once("source-link-updated"));

::: devtools/client/inspector/rules/views/rule-editor.js:30
(Diff revision 1)
> +loader.lazyRequireGetter(this, "EventEmitter",

Curious, is there any reason to use this over:

const EventEmitter = require("devtools/shared/event-emitter")
Attachment #8713191 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 6

3 years ago
(In reply to Brian Grinstead [:bgrins] from comment #5)
> ::: devtools/client/inspector/rules/rules.js:1163
> (Diff revision 1)
> > +        let onEditorReady = promise.defer();
> 
> Doesn't 'once' already return a Promise?  If so could you do:
> 
> editorReadyPromises.push(rule.editor.once("source-link-updated"));
Right, I missed that somehow. Fixed now
> ::: devtools/client/inspector/rules/views/rule-editor.js:30
> (Diff revision 1)
> > +loader.lazyRequireGetter(this, "EventEmitter",
> 
> Curious, is there any reason to use this over:
> 
> const EventEmitter = require("devtools/shared/event-emitter")
Hm, you're right, EventEmitter is used right away anyway, so there's no reason to lazy load it. Fixed.

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/223de53aa2dc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

3 years ago
Depends on: 1249888

Updated

3 years ago
Depends on: 1260510

Comment 10

3 years ago
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected

Updated

3 years ago
Depends on: 1263439

Updated

3 years ago
Depends on: 1255787
This has caused a few regressions:  bug 1249888 (fixed in 46+), bug 1255787 (only fixed in 48) and bug 1263439. Noting in case we see a pattern of more problems or in case they can be fixed in a more general way.

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.