1017751 - tab.onActivate can fire before the proper document exists (while still at about:blank) with e10s

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Tomislav Jovanovic :zombie]

(this is my understanding of what is happening, might not be 100% correct)

what happens is that when a new tab is opened, first an empty document (about:blank) is created and paired with a DOMWindow, and then it "navigates" to the actual desired URL. before this step is done, reading browser.currentURI returns 'about:blank'.

i believe even before e10s, this is an async process, but due to how events are triggered/ordered, by the time onActivate() addon code is executed, reading tab.url mostly works. 

with e10s however, this is no longer the case. TabSelect (activate) logic happens all in the parent process (possibly synchronously), while the document loading happens in the child, asynchronously. we have a few tests that fail because of this.

now i'm not sure this was ever a strong guarantee of our tabs api, though we have example code on MDN that shows reading tab.url on activate [1].

i can try hooking into WebProgressListener events [2], and delay firing onActivate until the actual document starts loading and the url property is correct, though i'm not sure how reliable this would be, and what would happen on errors and other corner-cases (maybe simply firing after a timeout would be enough to cover all of them?).

any thoughts on API guarantees/advice on solutions Irakly?


[1] https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/tabs#Track_a_single_tab
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebProgressListener

Comment 1

[Tomislav Jovanovic :zombie]

and i misspelled your name, sorry..  :(

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

If you only need currentURI to be correct for new tabs, we should be able to make that happen. Right now, if remoteBrowser.currentURI is requested and none has been set yet, we return about:blank. However, we should be able to change loadURI to set currentURI if it hasn't already been set yet. It should be a simple change here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/RemoteWebNavigation.jsm#62

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

So we talked over vidyo about this with :zombie and agreed that suggestion from Bill above is probably the best option to go with.

Comment 4

[Tomislav Jovanovic :zombie]

Bill, are you able to please take this one, as i'm not really set up to build/test firefox?

Comment 5

[Tomislav Jovanovic :zombie]

even with this fix, i still think we should update our documentation and example code to at least note that our API doesn't really guarantee a *new* tab will be ready in an "activate" event.

this will probably fix the most popular use case, but there will probably be corner-cases that will not work.

Will, what do you think? is there enough information in this bug, or do i need to file a separate docs bug?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here is a try push that includes the change I was considering:
https://tbpl.mozilla.org/?tree=Try&rev=cd8252c7d713

It will take a few hours for the build to complete. When it's done, if you click on the "B" button for your platform, and then on "go to build directory", you'll get an FTP link to a Firefox executable you can download. Please test it and see if it works. If it does, I'll ask for review on the patch.

Comment 7

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/tabs#activate_2, let me know if this is what you expect. If it isn't, let me know what you do expect. I don't think it's worth annotating that earlier example with this corner case.

Comment 8

[Tomislav Jovanovic :zombie]

(In reply to Will Bamberg [:wbamberg] from comment #7)
> I've updated https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/tabs#activate_2

thanks. this is good, though i filed bug 1026543 and would like to update that example to the proper way of handling this case, once we land that and expose tab.readyState.

Comment 9

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1524

depends on changes in bug 1026543

Comment 10

[Tomislav Jovanovic :zombie]

(In reply to Bill McCloskey (:billm) from comment #6)
> Please test it and see if it works. If it does, I'll ask for review on the
> patch.

hey Bill, thanks for this, though we went a different route. 

it's possible this change will still prove useful for another issue, but testing this requires other changes to land first. i'm gonna save this build to test later, so you don't have to keep track of that branch.

again, thank you for help and quick response..

Comment 11

[Tomislav Jovanovic :zombie]

hey Erik, this was the original bug that required exposing readyState in bug 1026543, so that's why i'm asking for review from you again.

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Tomislav Jovanovic [:zombie] from comment #10)
> (In reply to Bill McCloskey (:billm) from comment #6)
> > Please test it and see if it works. If it does, I'll ask for review on the
> > patch.
> 
> hey Bill, thanks for this, though we went a different route. 
> 
> it's possible this change will still prove useful for another issue, but
> testing this requires other changes to land first. i'm gonna save this build
> to test later, so you don't have to keep track of that branch.
> 
> again, thank you for help and quick response..

zombie could you please also comment what is the other route and why was it chosen ? Assuming it's a workaround, I'd suggest to pursue both landing Bill's patch and also landing our workaround in the meantime.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8447321 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1524

No need to request feedback for non API changes.

Comment 14

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c86ec149469f244ab174f705b3993d2df681ab92
bug 1017751 - activate before ready

https://github.com/mozilla/addon-sdk/commit/0f12cfa1c48f35c5829bcd66511693a79d567f4f
Merge pull request #1524 from zombie/1017751-ready-ativate

Bug 1017751 - tab.onActivate can fire before the proper document exists r=erikvold