Closed Bug 734835 Opened 12 years ago Closed 12 years ago

Failure in testBackgroundTabScrolling | Right scroll arrow has been highlighted

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox13 unaffected, firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr10 unaffected)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox13 --- unaffected
firefox14 --- fixed
firefox15 --- fixed
firefox16 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: remus.pop, Assigned: whimboo)

References

(
URL
)

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(1 file, 6 obsolete files)

Patch v5.1
2.26 KB, patch
davehunt
: review+
Details | Diff | Splinter Review 
The test fails only on Linux since 03.08.
This is reproducible and I can see that it stops opening tabs for some reason.
Is it a regression? ALso what do you mean with 03.08? I assume 8th of March?
Yes, that's right, it's 8th of March. 
I'm looking into changes merged to trunk from 7th to 8th of March to see if I can get any luck.
It's strange that it fails only on Linux.
I think bug 734814 will have the fix for this one as well.
Linux has a different behaviour of tabs.
Depends on: 734814
Whiteboard: [mozmill-test-failure]
If this is failing due to an intended change in Firefox, lets disable this test until bug 734814 is fix. If this is failing due to a bug in Firefox, we need to file a Firefox bug. This should be standard procedure (as per bug 734814 comment 15).
Remus, does the right arrow flash visually? I mean it could be that we do not catch the internal state of it because it's too quick. Thing is that we do not use an event listener yet.
It doesn't flash because not enough tabs are open. I think bug 734273 might be the issue here.
I've seen in Aurora on linux that the arrows appear with few tabs open.

An Aurora build with the fix is not out yet, but we should wait for it because this is a regression.
I can confirm this is fixed on Aurora as well, using the latest build.
Status: NEW → RESOLVED
Closed: 12 years ago
Depends on: 734273
Resolution: --- → FIXED
Verified that this test has stopped failing after March 12th.
Status: RESOLVED → VERIFIED
It's still failing in my CI for a ja build on Aurora:
http://mozmill-ci.blargon7.com/#/functional/report/e438d6e3916b2b636037d77445b27c9f
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Also failing in 12.0 Beta today:
http://mozmill-release.blargon7.com/#/functional/report/e438d6e3916b2b636037d77445ce6459

Remus, if this is a Firefox bug, please open a Firefox bug. If it's a bug in the test please disable the test until you can come up with a fix.
OS: Linux → Windows Vista
Attached patch skip test (beta) (obsolete) — — Splinter Review 
I hate doing this but I cannot reproduce locally so it's pretty hard to fix.
Attachment #606152 - Flags: review?(vlad.mozbugs)
Comment on attachment 606152 [details] [diff] [review]
skip test (beta)


> 
>-/**
>- * Map test functions to litmus tests
>- */
>-// testOpenInBackgroundTab.meta = {litmusids : [8259]};

Why are you removing this code block?
Attachment #606152 - Flags: review?(vlad.mozbugs) → review-
AFAIK, we no longer use comments regarding the litmus ID, so it's safe to remove them, but I may be wrong. Henrik could you please help?

Also, just saw that FIREFOX_12_0b1_BUILD1 with changeset 249ecd7beaf3 is now a close tree. We'll have to wait for new results from _BUILD2, so I'm canceling skipping until new data arrives.
AFAIK, we no longer use comments regarding the litmus ID, so it's safe to remove them, but I may be wrong. Henrik could you please help?
AFAIK, we no longer use comments regarding the litmus ID, so it's safe to remove them, but I may be wrong. Henrik could you please help?

Also, just saw that FIREFOX_12_0b1_BUILD1 with changeset 249ecd7beaf3 is now a close tree. We'll have to wait for new rseult from _BUILD2, so I'm canceling skipping until new data arrives.
(In reply to Remus Pop (:RemusPop) from comment #13)
> AFAIK, we no longer use comments regarding the litmus ID, so it's safe to
> remove them, but I may be wrong.

It has nothing to do with fixing this test. Only alter, add, or remove code which serves to fix the failure.

> Also, just saw that FIREFOX_12_0b1_BUILD1 with changeset 249ecd7beaf3 is now
> a close tree. We'll have to wait for new results from _BUILD2, so I'm
> canceling skipping until new data arrives.

Why does this matter? If this is a reproducible bug in Firefox 12.0b2 then a bug should be filed and this test should remain active so as to show the true failure. If this is a problem with the test then it matters not and should be disabled/fixed.
I no longer see this failing.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → INVALID
If it was an exiting failure it can't be invalid. Changing to WFM.
Resolution: INVALID → WORKSFORME
This is reoccurring, but this time on OSX too, besides Vista. Linux is unaffected.
URL: http://mozmill-release.blargon7.com/#... → http://mozmill-ci.blargon7.com/#/func...
Status: RESOLVED → REOPENED
OS: Windows Vista → All
Resolution: WORKSFORME → ---
It's somewhat intermittent. Vlad, can you please investigate this failure?
Assignee: nobody → vlad.mozbugs
URL: http://mozmill-ci.blargon7.com/#/func... → http://mozmill-ci.blargon7.com/#/func...
Status: REOPENED → ASSIGNED
Hardware: x86 → All
(In reply to Henrik Skupin (:whimboo) from comment #20)
> It's somewhat intermittent. Vlad, can you please investigate this failure?

On it
I am seeing the following error on Mac OS X 10.7 

ERROR | Test Failure: {"exception": {"stack": "TimeoutError@resource://mozmill/modules/utils.js:447\nwaitFor@resource://mozmill/modules/utils.js:485\n@resource://mozmill/modules/controller.js:685\n@resource://mozmill/modules/frame.js -> file:///Users/svuser/Desktop/fixFailureBackgroundTabScrolling/mozmill-tests/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js:71\n@resource://mozmill/modules/frame.js:563\n@resource://mozmill/modules/frame.js:632\n@resource://mozmill/modules/frame.js:675\n@resource://mozmill/modules/frame.js:512\n@resource://mozmill/modules/frame.js:687\n@resource://jsbridge/modules/server.js:184\n@resource://jsbridge/modules/server.js:188\n@resource://jsbridge/modules/server.js:288\n", "message": "Right scroll arrow has been highlighted", "fileName": "resource://mozmill/modules/utils.js", "name": "TimeoutError", "lineNumber": 447}}

1/3 runs the test fails with a timeout error so we are probably not waiting enough there. Moving on further
Or we miss that property change due to our delay in checking it. Probably we should listen for an event?
(In reply to Henrik Skupin (:whimboo) from comment #23)
> Or we miss that property change due to our delay in checking it. Probably we
> should listen for an event?

Will confirm that once I am certain. In both ways I would also recommend listening for an event
Well its certain alright...Henrik your assumption was right. We miss to check the property on time. 

The timeout for the property removal is set to 150 (see tabbrowser.xml to confirm) 

But adding an event listener will do the exact same thing as the .getNode().hasAttribute('notifybgtab') check we are doing. 

Suggestions are welcomed, guys
Well, right now we are polling for a change of the value, right? With an event listener for the overflow event we will get notified. It would be similar to the window close code we had before in utils.js.
Attached patch fix patch v1.0 (obsolete) — — Splinter Review 
Fix patch v1.0 

* Added an event listener for the "DOMAttrModified" event. The only attribute which is modified for our object is the scroll button animation attribute 
* Skip patch is made obsolete because we have a fix now
Attachment #606152 - Attachment is obsolete: true
Attachment #624338 - Flags: review?(hskupin)
Comment on attachment 624338 [details] [diff] [review]
fix patch v1.0

>+  // Add an event listener to catch scroll button animation
>+  scrollButtonDown.getNode().addEventListener("DOMAttrModified", function () {
>+    scrollButtonDownAnimation = true;
>+  }, false);

We do not want to add a listener for DOMAttrModified. Please see:
https://developer.mozilla.org/en/Extensions/Performance_best_practices_in_extensions#Avoid_DOM_mutation_event_listeners

Instead there should be an overflow event. You can ask Dao or check MXR if you need more information. I don't have that either at the moment. Also never forget to remove an event listener because that would cause leakage.
Attachment #624338 - Flags: review?(hskupin) → review-
You are so right

Well the "DOMAttrModified" was the only thing I could find to make this actually work at that time, I was somehow aware that its not that reliable :( 
Lets see if we can find a better event to listen for
Any update here? We kinda would like to see this intermittent failure fixed.
(In reply to Henrik Skupin (:whimboo) from comment #31)
> Any update here? We kinda would like to see this intermittent failure fixed.

I did lots of trying with the event you've recommended and found out that the overflow event is not something we want because its an event for tabstrip overflow and not for our case with the right scroll button animation. we would have to listen for that event and do the exact same check we did before..id does not help. 
I wanted to find some other solution to this case but no luck so far
Vlad, please ask Dao as already mentioned in comment 29. I'm sure he can help here.
cc-ing Dao 

Dao, do we have an event for the right scrolling arrow animation on the tabstrip? 
We need to listen for that event in this test, and you'd be a great help if you can offer me some guidance. Lots of thanks!
Vlad, please follow-up with dao on IRC or other channels. We really want to get this failure fixed.
(In reply to Henrik Skupin (:whimboo) from comment #35)
> Vlad, please follow-up with dao on IRC or other channels. We really want to
> get this failure fixed.
Just sent Dao an e-mail, hopefully he's got some time to help
(In reply to Dão Gottwald [:dao] from comment #37)
> As of Firefox 14, you can use this instead of DOMAttrModified:
> http://hacks.mozilla.org/2012/05/dom-mutationobserver-reacting-to-dom-
> changes-without-killing-browser-performance/

Tried to instantiate nsIDOMMutationObserver in the mozmill test with no luck. I can't find 
the interface in Ci or any class in Cc 

Ran this JS snippet 

var x;
  for (x in Cc)
  dump("\n*** " + x + "\n");

to get all the domain names. 

Dao - any ideas how to use the MutationObserver constructor in Mozmill? I mean I want to create the observer object with 'new MutationOberver' and I think I need to create an instance of nsIDOMMutationObserver for that...thanks :)
(In reply to Henrik Skupin (:whimboo) from comment #39)
> Vlad, any update here? It is failing again today:
> 
> http://mozmill-ci.blargon7.com/#/functional/report/
> e67171ea696231bb192f565615037a32

I cannot use DOMMutationObserver with Mozmill. Comment 38 explains the last thing I tried but without any luck. 
So the example in comment 37 is not applicable unless we find a way to override the MutationObserver contructor in Mozmill. 

Trying the example in comment 37 will make it clear what the problem is
Not sure what the problem here is. It's kinda easy to get a MutationObserver instantiated. It's accessible as 'controller.window.MutationObserver' as explained in the referenced blog post in comment 37.
(In reply to Henrik Skupin (:whimboo) from comment #42)
> Not sure what the problem here is. It's kinda easy to get a MutationObserver
> instantiated. It's accessible as 'controller.window.MutationObserver' as
> explained in the referenced blog post in comment 37.

I get "MutationObserver is undefined" 
You get instantiate controller.window.nsIDOMMutationObserver successfully but that is the XPCOM interface

I'll try something else and come up with the results shortly
Also, it writing 

var something = controller.window.MutationObserver(..) I get 

{"message": "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMMutationObserver.observe]", "name": "NS_ERROR_UNEXPECTED", "lineNumber": 87}}], "name": "testBackgroundTabScrolling.js::testScrollBackgroundTabIntoView", "filename": "/home/vladmaniac/Desktop/fixBackgroundScrolling/mozmill-tests/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js", "failed": 1, "passed": 49}
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #43)
> I get "MutationObserver is undefined" 
> You get instantiate controller.window.nsIDOMMutationObserver successfully
> but that is the XPCOM interface

Not sure what you are saying here. There is no nsIDOMMutationObserver interface property on the window object.

Works just fine for me:

  var observer = new controller.window.MutationObserver(function (mutations) {
      mutations.forEach(function (mutation) {
          console.log(mutation.type);
      });
  });
Attached patch patch v2.0 (obsolete) — — Splinter Review 
Fixed. 

Perhaps it will be better to check also that the attribute is the right one which is added, but there is not other addition in the DOM for the scrollButtonDown Object. 

At least MutationObserver works now - needed a clean repo :( 
Let me know about further changes needed
Attachment #624338 - Attachment is obsolete: true
Attachment #633438 - Flags: review?(hskupin)
Also, its probably better to add the MutationObserver in a shared module method, there are a few added code lines which complicates the test a bit
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #46)
> At least MutationObserver works now - needed a clean repo :( 

Not sure why you needed a clean repo here. Nothing should have changed. Can you elaborate why?

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #47)
> Also, its probably better to add the MutationObserver in a shared module
> method, there are a few added code lines which complicates the test a bit

Feel free to add a bug for it. We could even get this added to Mozmill proper.
Comment on attachment 633438 [details] [diff] [review]
patch v2.0

>+  // Check that the right scroll button flashes
>+  var insertedAttribute = false;

Please name it like 'hasHighlighted' so the test is better understandable.

>+  var observer = new controller.window.MutationObserver(function (mutations) {
>+    mutations.forEach(function (mutation) {
>+      insertedAttribute = true;
>+    });    

As you said yourself we want to wait for the right attribute. So please add that necessary code.

>+  var config = { attributes: true, childList: true, characterData: true };

Just move this up right before the MutationObserver instantiation.

>   controller.waitFor(function () {
>     return !scrollButtonDown.getNode().hasAttribute('notifybgtab');
>   }, "Hightlight should be removed immediately");

We will also have to use the MutationObserver here to be consistent.
Attachment #633438 - Flags: review?(hskupin) → review-
Attached patch patch v3.0 (obsolete) — — Splinter Review 
Fixed
Attachment #633438 - Attachment is obsolete: true
Attachment #633969 - Flags: review?(hskupin)
Please keep in mind that DOMMutationObservers are introduced since Firefox 14, so this would apply for the following branches: 

* default
* mozilla-aurora
* mozilla-beta 

and not for 

* mozilla-release at the moment being Firefox 13
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #51)
> Please keep in mind that DOMMutationObservers are introduced since Firefox
> 14, so this would apply for the following branches: 
> 
> * default
> * mozilla-aurora
> * mozilla-beta 

It would be more helpful to set the appropriate status flags. I will do that now. Also we never had failures with Firefox 13 or 10 ESR. So marking those branches as unaffected.
status-firefox-esr10: --- → unaffected
status-firefox13: --- → unaffected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
Comment on attachment 633969 [details] [diff] [review]
patch v3.0

>+  var hasHighlighted = false;
>+  var highlightRemoved = false;

Please make use of an object here like 'highlight = { set: false, unset: false }'

>+  var config = { attributes: true, childList: true, characterData: true };

Please check for the right config we have to use in our case. This is a copy&paste from the example above.

>+  var observeHighlight = new controller.window.MutationObserver(function (mutations) {
>+    mutations.forEach(function (mutation) {
>+      hasHighlighted = scrollButtonDown.getNode().hasAttribute('notifybgtab');
>+      highlightRemoved = !hasHighlighted;

This is not what you want to do due to the asynchronous handling of the code. Please see the W3 documentation how to work with MutationObervers correctly:

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers
Attachment #633969 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #53)
> Comment on attachment 633969 [details] [diff] [review]
> patch v3.0
> 
> >+  var hasHighlighted = false;
> >+  var highlightRemoved = false;
> 
> Please make use of an object here like 'highlight = { set: false, unset:
> false }'

Good point

> 
> >+  var config = { attributes: true, childList: true, characterData: true };
> 

Investigated and our case will need only attributes: true

> Please check for the right config we have to use in our case. This is a
> copy&paste from the example above.
> 
> >+  var observeHighlight = new controller.window.MutationObserver(function (mutations) {
> >+    mutations.forEach(function (mutation) {
> >+      hasHighlighted = scrollButtonDown.getNode().hasAttribute('notifybgtab');
> >+      highlightRemoved = !hasHighlighted;
> 
> This is not what you want to do due to the asynchronous handling of the
> code. Please see the W3 documentation how to work with MutationObervers
> correctly:
> 
> http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers

Still need to look into the documentation. Hopefully I'll fix it by eod
status-firefox-esr10: unaffected → ---
status-firefox13: unaffected → ---
status-firefox14: affected → ---
status-firefox15: affected → ---
status-firefox16: affected → ---
Flags were reset again, sorry - I can't explain why this is happening
> 
> This is not what you want to do due to the asynchronous handling of the
> code. Please see the W3 documentation how to work with MutationObervers
> correctly:
> 
> http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers

Are you suggesting here to record the old value of the attribute using attributeOldValue? In our case would be 'undefined' because the attribute does not exist at first, and after the event gets fired it is created
status-firefox-esr10: unaffected → ---
status-firefox13: unaffected → ---
status-firefox14: affected → ---
status-firefox15: affected → ---
status-firefox16: affected → ---
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #56)
> Are you suggesting here to record the old value of the attribute using
> attributeOldValue? In our case would be 'undefined' because the attribute
> does not exist at first, and after the event gets fired it is created

You cannot expect that this attribute doesn't exist! We are running other tests before this one, so it can happen at any time. I'm fairly sure the attribute which has been changed comes with the mutation object.
(In reply to Henrik Skupin (:whimboo) from comment #57)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #56)
> > Are you suggesting here to record the old value of the attribute using
> > attributeOldValue? In our case would be 'undefined' because the attribute
> > does not exist at first, and after the event gets fired it is created
> 
> You cannot expect that this attribute doesn't exist! We are running other
> tests before this one, so it can happen at any time. I'm fairly sure the
> attribute which has been changed comes with the mutation object.

We can get the attribute name by 'mutation.attributeName' and we can assert for the highlight attribute. Now need to figure out how do we check that the highlight disappears
status-firefox-esr10: unaffected → ---
status-firefox13: unaffected → ---
status-firefox14: affected → ---
status-firefox15: affected → ---
status-firefox16: affected → ---
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #58)
> We can get the attribute name by 'mutation.attributeName' and we can assert
> for the highlight attribute. Now need to figure out how do we check that the
> highlight disappears

In this case we probably also get the value via attributeValue? That one you will have to evaluate.
> In this case we probably also get the value via attributeValue? That one you
> will have to evaluate.

mutations.attributeValue is 'undefined' 
also, the usage is not documented here 

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers
Attached patch patch v4.0 (obsolete) — — Splinter Review 
This time we are getting the attribute name from 'mutation.attributeName' so we are sure its the right one. 

We test for insertion of the attribute and for extraction of it, by 
element.getNode().hasAttribute(mutation.attributeName) 

We have it right this time
Attachment #633969 - Attachment is obsolete: true
Attachment #634426 - Flags: review?(hskupin)
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #60)
> > In this case we probably also get the value via attributeValue? That one you
> > will have to evaluate.
> 
> mutations.attributeValue is 'undefined' 
> also, the usage is not documented here 

This was an example which you shouldn't take 1-1. I don't want to specify all the steps you will have to do to get this fixed, but want to show directions. So please check how to get the attribute value.

Also see:
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-attribute

"Attributes have a name and value."
> "Attributes have a name and value."

Just thought I could somehow get the value from the mutation object. my proposed patch is uploaded; lots of thanks for guidance in this one
Comment on attachment 634426 [details] [diff] [review]
patch v4.0

>+  var observeHighlight = new controller.window.MutationObserver(function (mutations) {
>+    mutations.forEach(function (mutation) {
>+      highlight.set = mutation.attributeName;
>+      highlight.unset = scrollButtonDown.getNode().hasAttribute(mutation.attributeName);
>+    });    

So once again. This observer is called twice for highlight and normal state. So this will not work. Also you can only evaluate the values if the mutation you get here is for the wanted attribute. Right now you are reacting on any attribute change.

So before you continue in trying to put a patch together, can you please come up with some pseudo-code how you would solve it?
Attachment #634426 - Flags: review?(hskupin) → review-
I am reacting on any attribute change and save the attribute name, then verify I have inserted the correct attribute. 

I cannot react on the specific attribute because the attribute does not exist all the time and would trow an exception. I need to observe the element's node for attribute changes and not the attribute for changes. that is not possible in this case. 

As well as for the observer being called twice, I agree we are saving useless data, so my idea is to create two observer instances, one for insertedHighlight and one for removedHighlight 

My pseudocode: http://themaniak.pastebin.mozilla.org/1668530
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #65)
> I am reacting on any attribute change and save the attribute name, then
> verify I have inserted the correct attribute. 

The two steps way is all not necessary. Only evaluate the attribute if it's the bghighlight one as given by the mutation.

> I cannot react on the specific attribute because the attribute does not
> exist all the time and would trow an exception. I need to observe the
> element's node for attribute changes and not the attribute for changes. that
> is not possible in this case. 

When the attribute changes I'm fairly sure that we also get the value via the mutation object. If you are not sure about please contact the developer who has implemented this feature in Firefox. I do not think that it is save to check the node for the attribute value at the time the mutation observer notifies us. Checking the node's attribute can already be too late and the attribute value could have been changed or the attribute been removed.

> As well as for the observer being called twice, I agree we are saving
> useless data, so my idea is to create two observer instances, one for
> insertedHighlight and one for removedHighlight 

Please think about the event flow. This proposal doesn't work because we will not be able to catch the second mutation notification. It's registered too late. Please use a single mutation observer. And also make sure to release it afterward to not cause a memory leak.

> My pseudocode: http://themaniak.pastebin.mozilla.org/1668530

Again, using external pages it not that helpful because content will be lost over time. Always add comments or attach a document to the bug. In cases like those comments are very helpful because you can query for those on Bugzilla.
> 
> Please think about the event flow. This proposal doesn't work because we
> will not be able to catch the second mutation notification. It's registered
> too late. Please use a single mutation observer. And also make sure to
> release it afterward to not cause a memory leak.
> 
I agree, that's why I used a single mutation observer in the first place. 
The thing is that the attribute value does not change at all. 
At the overflow of the tabstrip, the attribute "notifybgtab" is inserted in the node with the value = true. After a few moments, its being removed completely, but the value does not change. its just removed. the MutationObserver is sensing the insertion and deletion of the attribute, we can easily verify this by dumping mutations.type in the console, it will echo "attribute" twice. I can test that the attribute is inserted by verifying the attributeName, but I'm not sure how to deal with the attribute removal.
Currently waiting for dao's feedback on this
Attached patch Patch v5 (obsolete) — — Splinter Review 
So we shouldn't wait any longer on this bug. Given the slow pace I have taken the stab and modified the test so that the short highlight should be covered safely now.

There is one issue with latest Nightly builds, which have to be checked. MutationObserver doesn't seem to work reliable when Firefox is in the background. Especially in such a case we miss the first transition.

Vlad, would you be so kind to check for a regression range? The latest Aurora build is still working fine.
Assignee: vlad.mozbugs → hskupin
Attachment #639053 - Flags: review?(dave.hunt)
Attachment #634426 - Attachment is obsolete: true
> There is one issue with latest Nightly builds, which have to be checked.
> MutationObserver doesn't seem to work reliable when Firefox is in the
> background. 

Do we have a bug for this or its something you've noticed while testing your patch?
There is no bug existent yet for that issue. Having a quick check for a regression range would help me to file a better bug report about that regression.
(In reply to Henrik Skupin (:whimboo) from comment #71)
> There is no bug existent yet for that issue. Having a quick check for a
> regression range would help me to file a better bug report about that
> regression.

I see. Can I have a testcase to reproduce what you are seeing? 

Right now I did: 

1. import your patch 
2. ran testTabbedBrowsing/testBackroundScrolling.js locally 
3. put Firefox in the background by triggering an OS event which is more prior in Ubuntu, like window switching

result: 
I got this error 
"message": "The all tabs popup should have been opened", "fileName": "resource://mozmill/modules/utils.js", "name": "TimeoutError", "lineNumber": 447}}],

which is not the one related to our mutation observing code
The updated waitFor method should have been failed. Not sure what you mean by 'triggering an OS event'. Simply select the terminal again right after you have issued the command.
(In reply to Henrik Skupin (:whimboo) from comment #73)
> The updated waitFor method should have been failed. Not sure what you mean
> by 'triggering an OS event'. Simply select the terminal again right after
> you have issued the command.

Not failing for me, sorry. What failure message do you get?
Comment on attachment 639053 [details] [diff] [review]
Patch v5

Looks great!
Attachment #639053 - Flags: review?(dave.hunt) → review+
Attached patch Patch v5.1 — — Splinter Review 
We can even make it simpler. There is no need to check both states. The important one is the transition which removes the highlight. It will contain the old value of the attribute which we need to determine if a flash happened. With that we can even detect if another unwanted transition happens afterward.

Also this will kill the above mentioned problem when the mutation for the highlight is not getting caught. I will investigate that on another bug.
Attachment #639053 - Attachment is obsolete: true
Attachment #639088 - Flags: review?(dave.hunt)
Comment on attachment 639088 [details] [diff] [review]
Patch v5.1

Even better! :)
Attachment #639088 - Flags: review?(dave.hunt) → review+
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/0ea5dabb151d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
status-firefox16: affected → fixed
Resolution: --- → FIXED
This happened again today on ESR Vista.
http://mozmill-ci.blargon7.com/#/functional/report/2e7486a8c08b5541b9074117e08dac14

Should we reopen this bug or open a new one?
Looks like we should also land it on the esr10 branch.
status-firefox-esr10: unaffected → affected
Well, Alex please file a new bug. It could be something different. As we have seen some months ago esr10 was not affected.
status-firefox-esr10: affected → fixed
(In reply to Henrik Skupin (:whimboo) from comment #82)
> Well, Alex please file a new bug. It could be something different. As we
> have seen some months ago esr10 was not affected.
Logged bug 803581
Depends on: 804561
Not sure why but the patch landed also on esr10 by accident. It broke the test completely because MutationObserver is not available there.

I backed out the appropriate changeset:
http://hg.mozilla.org/qa/mozmill-tests/rev/44217a27492e
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: