917834 - require("sdk/tabs").open doesn't work correctly

Status: RESOLVED INCOMPLETE

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

Description

[Tiziana Sellitto [:tiziana]]

After updating Firefox Beta to 25.0 yesterday (buildID:20130917123208) I've noticed that all the links in my extension that open a new tab using tabs.open don't work correctly. You need to click on them 3/4 times, even more, before they open the tab but sometimes they don't open the new tab at all.

It works fine on Firefox 24 official release (buildID:20130910160258) and it worked fine in Firefox beta until the latest update.

In the URL an example I made, hoping it can help.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Tiziana, will look into this and see what is the issue; thanks!

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

It looks like because the lack of any value in the `href` of the anchors, the panel is reloaded upon click, as the event isn't stopped, or not reloading the same page (like with a href="#") -- this behaviour is also seen just by opening the index.html page (and removing the addon.port line), and you can see the page is reloaded immediately on clicking. It looks like Firefox's behaviour changed for this at v25 -- will look into if this is intentional or not!

In the meantime, adding href="#" to your links will fix the issue

Comment 3

[Tiziana Sellitto [:tiziana]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> In the meantime, adding href="#" to your links will fix the issue

Thanks a lot!

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

When automating tests for this issue, it works, because the click occurs fast enough before the page can reload and the worker is destroyed. Same when you use the sample case above in the URL, if you click fast, you can trigger the tabs opening before the page is reloaded. The panel changes made a few weeks ago now cause this issue, but not sure if this is something we can fix due to the race-condition nature of the reload and worker communication without leaving workers hanging around after the DOM is destroyed.

Irakli, thoughts?

Comment 5

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4)
> When automating tests for this issue, it works, because the click occurs
> fast enough before the page can reload and the worker is destroyed. Same
> when you use the sample case above in the URL, if you click fast, you can
> trigger the tabs opening before the page is reloaded. The panel changes made
> a few weeks ago now cause this issue, but not sure if this is something we
> can fix due to the race-condition nature of the reload and worker
> communication without leaving workers hanging around after the DOM is
> destroyed.
> 
> Irakli, thoughts?

Can you point out panel changes you are talking about ?

I have not verified in specs, but I don't expect href="" is supposed to reload a page. Neither it
does in my simple test on the regular tab. I think we should fix regression in panel to make sure
it does not happens. Also I don't think it needs to be p1 since it's easy enough to avoid this issue.

If this issues is reproducible with any specific URL that will be a more severe issue. Although I don't expect that to be a case, because all the queued up messages are delivered synchronously if worker is being destroyed.

Removing p1 so we can re-triage this one again.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Thought it was the refactor from here: https://github.com/mozilla/addon-sdk/commit/9bb40faeecf1de3974ff6f16625ea23d3d66e9af
But that looks like it landed in 24.

According to RFC 2396, a blank href should refer to the current document (http://stackoverflow.com/a/5637990). While not common (I had no idea it was valid), it is part of the spec.

No big changes in panel post that refactor, so maybe something with the content workers caused this.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Confirmed this works fine with other http urls (http://google.com, http://mozilla.org), and '#' like mentioned previously -- it's only the blank href that causes the race

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Unassigning myself from bugs I haven't gotten around to

Comment 9

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562