Closed Bug 1212593 Opened 9 years ago Closed 9 years ago

Intermittent test_register.html | could not register for push notification | Some test failed with error TypeError: pushSubscription is null

Categories

(Core :: DOM: Push Subscriptions, defect)

Product:
Core
Component:
DOM: Push Subscriptions
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: KWierso, Assigned: dragana)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

bug_1212593.patch
6.21 KB, patch
lina
: review+
Details | Diff | Splinter Review
Flags: needinfo?(kcambridge)
Flags: needinfo?(MattN+bmo)
I'm not the right person to look at this (see bug 1191453 comment 22)
Flags: needinfo?(MattN+bmo) → needinfo?(martin.thomson)
Blocks: 1206969
something changed on 7. Oct. that triggers this error.
Anyway I will make a patch that will fix this.
Assignee: nobody → dd.mozilla
Attached patch bug_1212593.patch (obsolete) — — Splinter Review 
When I was implementing PushService, we decided that we do not need to switch between different push servers during runtime. I think this is still true.
I have implemented this possibility only because it is needed for test(so test can change server at the beginning). I could fix it to be safe also for normal runtime, but if it is not needed I will leave it like this. 

This patch is going to fix failing test an also test_try_registering_offline_disabled.html which is disabled
Attachment #8674923 - Flags: review?(kcambridge)
Comment on attachment 8674923 [details] [diff] [review]
bug_1212593.patch

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

Looks good! Just one question about the starting case.

It's so bizarre that `dom.push.connection.enabled` has anything to do with `test_register.html` failing. We never change it in that test, do we? Is that because `nsPref:changed` or `network:offline-status-changed` is fired at startup?

::: dom/push/PushService.jsm
@@ +362,2 @@
>        }
>        case CHANGING_SERVICE_EVENT:

Does the `STARTING_SERVICE_EVENT` case above need the same change?
Attachment #8674923 - Flags: review?(kcambridge)
Status: NEW → ASSIGNED
Flags: needinfo?(martin.thomson)
Flags: needinfo?(kcambridge)
(In reply to Kit Cambridge [:kitcambridge] from comment #15)
> Comment on attachment 8674923 [details] [diff] [review]
> bug_1212593.patch
> 
> Review of attachment 8674923 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Just one question about the starting case.
> 
> It's so bizarre that `dom.push.connection.enabled` has anything to do with
> `test_register.html` failing. We never change it in that test, do we? Is
> that because `nsPref:changed` or `network:offline-status-changed` is fired
> at startup?
> 

The problem is that the test before test_register.html is test_multiple_register_during_service_activation.html and this test changes the dom.push.serverURL pref twice. Our test suite changes this pref back also twice, like rewind. And if this happens fast enough it is finished before next test starts and everything is fine (that is why it never happens on my computer). But if this does not happen fast enough: 2 pref change are queued, the next test starts and queues a register request(because pusheService is in the activation state), the first pref change finishes, changes the state to active, so the register request starts to be handle and than the next 
pref change starts which deletes service...
I change that if a new pref change event comes during one is handled, they are both handled before the state is change to active.

This pref is not suppose to be change during normal runtime only for tests.

> ::: dom/push/PushService.jsm
> @@ +362,2 @@
> >        }
> >        case CHANGING_SERVICE_EVENT:
> 
> Does the `STARTING_SERVICE_EVENT` case above need the same change?

I will change this too, but this will not cause problem.

        Attachment #8674923 -
        Attachment is obsolete: true

        Attachment #8676085 -
        Flags: review?(kcambridge)

    
    

    
  
Is it possible to register a callback for the pref change and await that toward the end of the test?  That way you can ensure that the pref change takes effect before the test exits.

https://dxr.mozilla.org/mozilla-central/source/dom/push/test/test_multiple_register_during_service_activation.html#62
var resolvePrefChange;
var ensurePrefChangeCompleted = new Promise(r => resolvePrefChange = r);
SpecialPowers.pushPrefEnv({"set": [["dom.push.serverURL", defaultServerURL]]},
                          resolvePrefChange);

https://dxr.mozilla.org/mozilla-central/source/dom/push/test/test_multiple_register_during_service_activation.html#98-100
    .catch(err => {
      ok(false, "Some test failed with error " + err);
    }).then(_ => ensurePrefChangeCompleted).then(SimpleTest.finish);

    
    

    
      
  

    
    

    
  
Comment on attachment 8676085 [details] [diff] [review]
bug_1212593.patch

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

Cool, thanks for the explanation, Dragana!

> Is it possible to register a callback for the pref change and await that
> toward the end of the test?  That way you can ensure that the pref change
> takes effect before the test exits.

`SpecialPowers.popPrefEnv` might be useful, too. Let's land this patch as-is, then tackle waiting for the pref change in a follow-up bug.

        Attachment #8676085 -
        Flags: review?(kcambridge) → review+

    
    

    
  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3b01168df0c

the push test failure i this try run are another bug.

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2f41484b6005
Status: ASSIGNED → RESOLVED
Closed: 9 years ago

          status-firefox44:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

    
    

    
      
  

    
    

    
      
  

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: