Closed Bug 1378133 Opened 7 years ago Closed 7 years ago

Empty Inspector after toggling through some websites in the same tab

Categories

(DevTools :: Inspector, defect)

53 Branch
defect
Not set
normal

Tracking

(firefox-esr52 unaffected, firefox54 wontfix, firefox55 wontfix, firefox56 verified)

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- verified

People

(Reporter: bmaris, Assigned: ochameau)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Affected versions]:
- Firefox 54.0.1 RC
- Firefox 55 beta 6
- latest Nightly 56.0a1

[Affected platforms]:
- Ubuntu 16.04 32bit
- Windows 10.64bit
- macOS 10.12.5

[Steps to reproduce]:
1. Start Firefox
2. Visit https://www.mozilla.org/en-US/developer/css-grid/
3. Open Inspector
4. In the same tab go to another website (eg: wikipedia.org)
5. Hit back button

[Expected result]:
- Content can be seen inside inspector.

[Actual result]:
- Inspector is completely blank.

[Regression range]:
- This is a regression, but an old one.

Last good revision: a105cf2d33a5643d63c516d5ecfe917d8eb5eeee
First bad revision: fb4f5c7e082a4176cd1b8f3c784e5f424417e3fa
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a105cf2d33a5643d63c516d5ecfe917d8eb5eeee&tochange=fb4f5c7e082a4176cd1b8f3c784e5f424417e3fa

Culprit bug: Bug 1151909 - The markup view doesn't initialize until the page is fully loaded but it should be able to open on DOMContentLoaded

[Additional notes]:
- I did not see this issue on other website, there could be more affected.
Flags: needinfo?(poirot.alex)
Given that there is no plan to ship another dot release and this is not a new regression, mark 54 won't fix and see if we can have the fix in 55.
This is related to the bfcache. It will only appear when the first page you are loading (here, mozilla.org one) is "bfcached". It only happens if various conditions are met, you can find them in this page:
  https://developer.mozilla.org/en-US/Firefox/Releases/1.5/Using_Firefox_1.5_caching

So a more generic STR is this one:
* open a "bfcachable" page
* open the inspector
* navigate to any other page
* go back

And the inspector ends up being blank.

Thanks a lot for identifying this regression Bogdan!
I'm wondering how did you ended up discovering it? While testing another patch?
I'm looking forward ways to uncover such regression sooner.
Flags: needinfo?(poirot.alex)
Wait, it isn't just about being bfcachable. We already have a test covering that (devtools/client/inspector/test/browser_inspector_navigation.js).
For some reason, the css-grid page introduce some unexpected behavior while being cached.
When a page goes into the bfcache, a "pagehide" event is fired with "persisted=true" attribute:
  http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#1556-1565

The page goes into the cache when we load wikipedia page.
Then, when we go back to the css-grid page, we should receive a "pageshow" event, with "persisted=true" attribute:
  http://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#1536-1541

We get the expected "pagehide" event, but the "pageshow" one is fired with "persisted=false", which introduces this bug.

I'm able to write a patch to accomodate that, but I don't know how to reproduce that via a test...
Comment on attachment 8883484 [details]
Bug 1378133 - Fix the inspector when loading pages from bfcache.

https://reviewboard.mozilla.org/r/154386/#review159534

Thanks for fixing this! The patch works and I'm not sure how fragile this area is, so I am okay with landing as is.
I really feel like the logic is a bit shaky at the moment, but I don't think we have enough tests to refactor this confidently.

More importantly, I tried investigating the use caseof https://www.mozilla.org/en-US/developer/css-grid/. I isolated the issue and it seems to come from the srcset property of the img tag. 

> <!doctype html>
> <html>
>   <img src="src.jpg" srcset="srcset.jpg 1.5x">
> </html>

=> will trigger the issue. If you remove the srcset attribute it won't. (and it doesn't actually matter if srcset points to a valid url or not, it will result in a blank inspector in both cases)

For such a page, we get a "pageshow" event with persisted=false, but no DOMWindowCreated event.

We can add such an img tag to devtools/client/inspector/test/doc_inspector_breadcrumbs.html and it will test your fix. 
But I think the best would be to file a bug to fix what seems to me like a bfcache bug.

::: devtools/server/actors/tab.js:1545
(Diff revision 1)
>      // prior DOMWindowCreated event. For persisted pages, act as if the window
>      // had just been created since it's been unfrozen from bfcache.
> -    if (evt.type == "pageshow" && !evt.persisted) {
> +    // For some unexpected reason, some bfcached pages like
> +    // https://www.mozilla.org/en-US/developer/css-grid/
> +    // do not fire pageshow with persisted=true, instead, persisted is false.
> +    // Special case that by looking into know windows.

nit: know -> known

::: devtools/server/actors/tab.js:1546
(Diff revision 1)
> +    if (evt.type == "pageshow" &&
> +        !evt.persisted &&
> +        this._knownWindowIDs.has(innerID)
> +       ) {
>        return;
>      }
>  
> -    let window = evt.target.defaultView;
>      this._tabActor._windowReady(window);
>  
>      if (evt.type !== "pageshow") {
> -      this._knownWindowIDs.set(getWindowID(window), window);
> +      this._knownWindowIDs.set(innerID, window);
>      }

I would love it if we could simplify this as 

> if (knownIDs.has(id)) { return; }
> windowReady(window); 
> knownIDs.set(id, window); 

In theory it should be fine because we only listen to DOMWindowCreated and pageshow. DOMWindowCreated is supposed to be fired before pageshow, and for bfcached pages, only pageshow is fired. So rather than relying on the event types, we could simply rely on our knownIDs map to know what to do. 

But I'm afraid we might run into other race condition issues.

::: devtools/server/actors/tab.js:1555
(Diff revision 1)
>      }
>  
> -    let window = evt.target.defaultView;
>      this._tabActor._windowReady(window);
>  
>      if (evt.type !== "pageshow") {

Not related to your patch, but isn't it a bit weird that for bfcached pages we never set the window id in this map (since all we get are pageshow events)

::: devtools/server/actors/tab.js:1575
(Diff revision 1)
>        return;
>      }
>  
>      let window = evt.target.defaultView;
>      this._tabActor._windowDestroyed(window, null, true);
> +    this._knownWindowIDs.delete(getWindowID(window));

I think it makes sense to do that, but as I commented above, the ids for bfcached windows are never set in the map, so this is always a noop.
Attachment #8883484 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #5)
>
> More importantly, I tried investigating the use caseof
> https://www.mozilla.org/en-US/developer/css-grid/. I isolated the issue and
> it seems to come from the srcset property of the img tag. 
> 
> > <!doctype html>
> > <html>
> >   <img src="src.jpg" srcset="srcset.jpg 1.5x">
> > </html>

Wha, thanks for figuring this out. It looks like it has already been reported in bug 1340592.
 
> I would love it if we could simplify this as 
> 
> > if (knownIDs.has(id)) { return; }
> > windowReady(window); 
> > knownIDs.set(id, window); 
> 
> In theory it should be fine because we only listen to DOMWindowCreated and
> pageshow. DOMWindowCreated is supposed to be fired before pageshow, and for
> bfcached pages, only pageshow is fired. So rather than relying on the event
> types, we could simply rely on our knownIDs map to know what to do. 
> 
> But I'm afraid we might run into other race condition issues.

That should be fine. We already had a test covering the bfcache scenario,
at the end we missed this very special "srcset" usecase.

> ::: devtools/server/actors/tab.js:1555
> (Diff revision 1)
> >      }
> >  
> > -    let window = evt.target.defaultView;
> >      this._tabActor._windowReady(window);
> >  
> >      if (evt.type !== "pageshow") {
> 
> Not related to your patch, but isn't it a bit weird that for bfcached pages
> we never set the window id in this map (since all we get are pageshow events)

Before that patch the window wasn't removed from the knownWindowIDs on pagehide,
when going to the bfcache, so that was fine. They should have the same ID when coming back from the bfcache if I remember correctly.

> 
> ::: devtools/server/actors/tab.js:1575
> (Diff revision 1)
> >        return;
> >      }
> >  
> >      let window = evt.target.defaultView;
> >      this._tabActor._windowDestroyed(window, null, true);
> > +    this._knownWindowIDs.delete(getWindowID(window));
> 
> I think it makes sense to do that, but as I commented above, the ids for
> bfcached windows are never set in the map, so this is always a noop.

But you are right, with my current patch, this is wrong, we should re-add them on pageshow.
you suggested code where we just look if the ID is in the map during pageshow should fix that.
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> This is related to the bfcache. It will only appear when the first page you
> are loading (here, mozilla.org one) is "bfcached". It only happens if
> various conditions are met, you can find them in this page:
>  
> https://developer.mozilla.org/en-US/Firefox/Releases/1.5/Using_Firefox_1.
> 5_caching
> 
> So a more generic STR is this one:
> * open a "bfcachable" page
> * open the inspector
> * navigate to any other page
> * go back
> 
> And the inspector ends up being blank.
> 
> Thanks a lot for identifying this regression Bogdan!
> I'm wondering how did you ended up discovering it? While testing another
> patch?
> I'm looking forward ways to uncover such regression sooner.

Don't know if this helps much but I found this bug while testing CSS Grid Inspector. 
We have a series of tests there and one of them was to switch between websites and see if the Grid overlay will appear only on the page that uses CSS Grid.
Comment on attachment 8883484 [details]
Bug 1378133 - Fix the inspector when loading pages from bfcache.

https://reviewboard.mozilla.org/r/154386/#review165726

::: devtools/client/inspector/test/browser_inspector_navigation.js:17
(Diff revision 2)
>  
>  const TEST_URL_1 = "http://test1.example.org/" + TEST_URL_FILE;
>  const TEST_URL_2 = "http://test2.example.org/" + TEST_URL_FILE;
>  
> +const TEST_URL_3 = "data:text/html;charset=utf-8," +
> +  encodeURIComponent("<img src=\"foo.png\" srcset=\"foo.png 1.5x\" />");

Add a comment to explain why this TEST URL is relevant (link to other bug)
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4c1365ff941
Fix the inspector when loading pages from bfcache. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/b4c1365ff941
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Given where we are in the cycle, I think we should wontfix this for 55. DevEdition will pick up the fix in less than two weeks anyway once 56 gets uplifted to Beta. If you feel otherwise, however, feel free to change the status back to affected and nominate the patch for approval.
Assignee: nobody → poirot.alex
Blocks: 1151909
Version: Trunk → 53 Branch
I have reproduced this bug with Nightly 56.0a1 (2017-07-04) on Windows 8.1 ! 

This bug's fix is Verified with latest Beta 56.0b9 !

Build   ID    20170903140023
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

[testday-20170901]
I have reproduced this bug with Nightly 56.0a1 (2017-07-04) (64-bit) on Ubuntu 16.04 LTS! 

This bug's fix is Verified with latest Beta!

Build ID   :  20170911193316
User Agent :  Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170913]
as par comment 15 & comment 16 , i am marking this bug as verified fixed .
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.