Closed Bug 1017751 Opened 11 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

(e10s+)

RESOLVED FIXED
Tracking Status
e10s + ---

People

(Reporter: zombie, Assigned: zombie)

References

Details

Attachments

(1 file)

(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
and i misspelled your name, sorry.. :(
Blocks: 1015623
Flags: needinfo?(rFobic)
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
So we talked over vidyo about this with :zombie and agreed that suggestion from Bill above is probably the best option to go with.
Flags: needinfo?(rFobic)
Bill, are you able to please take this one, as i'm not really set up to build/test firefox?
Flags: needinfo?(wmccloskey)
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?
Flags: needinfo?(wbamberg)
Priority: -- → P1
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.
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wbamberg) → needinfo?(tomica+amo)
Depends on: 1026543
(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.
Flags: needinfo?(tomica+amo)
depends on changes in bug 1026543
Assignee: nobody → tomica+amo
Status: NEW → ASSIGNED
Attachment #8447321 - Flags: review?(evold)
(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..
Attachment #8447321 - Flags: feedback?(rFobic)
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.
Attachment #8447321 - Flags: review?(evold) → review+
(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 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.
Attachment #8447321 - Flags: feedback?(rFobic)
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: